[ovs-dev] [threaded-learning 10/25] ofproto: Break actions out of rule into new rule_actions structure.

Ethan Jackson ethan at nicira.com
Thu Sep 12 01:51:11 UTC 2013


My only comment is that you might consider using thread safety
annotations to force users do un-reference actions they've taken a
reference of.  That might be a pain though, not sure if it's worth it.

Acked-by: Ethan Jackson <ethan at nicira.com>


On Tue, Sep 10, 2013 at 10:27 PM, Ben Pfaff <blp at nicira.com> wrote:
> This permits code to ensure long-term access to a rule's actions
> without holding a long-term lock on the rule's rwlock.
>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  ofproto/connmgr.c            |    4 +-
>  ofproto/ofproto-dpif-xlate.c |   16 +++--
>  ofproto/ofproto-dpif.c       |   26 +++++---
>  ofproto/ofproto-dpif.h       |    4 +-
>  ofproto/ofproto-provider.h   |   28 ++++++++-
>  ofproto/ofproto.c            |  140 ++++++++++++++++++++++++++++--------------
>  6 files changed, 151 insertions(+), 67 deletions(-)
>
> diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
> index 2f315e6..1edbd59 100644
> --- a/ofproto/connmgr.c
> +++ b/ofproto/connmgr.c
> @@ -1903,8 +1903,8 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule,
>                  ovs_mutex_unlock(&rule->timeout_mutex);
>
>                  if (flags & NXFMF_ACTIONS) {
> -                    fu.ofpacts = rule->ofpacts;
> -                    fu.ofpacts_len = rule->ofpacts_len;
> +                    fu.ofpacts = rule->actions->ofpacts;
> +                    fu.ofpacts_len = rule->actions->ofpacts_len;
>                  } else {
>                      fu.ofpacts = NULL;
>                      fu.ofpacts_len = 0;
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index e7cec14..eb6a1f9 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -44,6 +44,7 @@
>  #include "ofproto/ofproto-dpif-mirror.h"
>  #include "ofproto/ofproto-dpif-sflow.h"
>  #include "ofproto/ofproto-dpif.h"
> +#include "ofproto/ofproto-provider.h"
>  #include "tunnel.h"
>  #include "vlog.h"
>
> @@ -1671,8 +1672,7 @@ xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif *rule)
>      OVS_RELEASES(rule)
>  {
>      struct rule_dpif *old_rule = ctx->rule;
> -    const struct ofpact *ofpacts;
> -    size_t ofpacts_len;
> +    struct rule_actions *actions;
>
>      if (ctx->xin->resubmit_stats) {
>          rule_dpif_credit_stats(rule, ctx->xin->resubmit_stats);
> @@ -1680,8 +1680,9 @@ xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif *rule)
>
>      ctx->recurse++;
>      ctx->rule = rule;
> -    rule_dpif_get_actions(rule, &ofpacts, &ofpacts_len);
> -    do_xlate_actions(ofpacts, ofpacts_len, ctx);
> +    actions = rule_dpif_get_actions(rule);
> +    do_xlate_actions(actions->ofpacts, actions->ofpacts_len, ctx);
> +    rule_actions_unref(actions);
>      ctx->rule = old_rule;
>      ctx->recurse--;
>
> @@ -2528,6 +2529,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>      struct flow_wildcards *wc = &xout->wc;
>      struct flow *flow = &xin->flow;
>
> +    struct rule_actions *actions = NULL;
>      enum slow_path_reason special;
>      const struct ofpact *ofpacts;
>      struct xport *in_port;
> @@ -2604,7 +2606,9 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>          ofpacts = xin->ofpacts;
>          ofpacts_len = xin->ofpacts_len;
>      } else if (xin->rule) {
> -        rule_dpif_get_actions(xin->rule, &ofpacts, &ofpacts_len);
> +        actions = rule_dpif_get_actions(xin->rule);
> +        ofpacts = actions->ofpacts;
> +        ofpacts_len = actions->ofpacts_len;
>      } else {
>          NOT_REACHED();
>      }
> @@ -2690,4 +2694,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>
>  out:
>      ovs_rwlock_unlock(&xlate_rwlock);
> +
> +    rule_actions_unref(actions);
>  }
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 97735e2..cbaae5a 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4162,12 +4162,13 @@ facet_is_controller_flow(struct facet *facet)
>          bool is_controller;
>
>          rule_dpif_lookup(ofproto, &facet->flow, NULL, &rule);
> -        ofpacts_len = rule->up.ofpacts_len;
> -        ofpacts = rule->up.ofpacts;
> +        ofpacts_len = rule->up.actions->ofpacts_len;
> +        ofpacts = rule->up.actions->ofpacts;
>          is_controller = ofpacts_len > 0
>              && ofpacts->type == OFPACT_CONTROLLER
>              && ofpact_next(ofpacts) >= ofpact_end(ofpacts, ofpacts_len);
>          rule_dpif_release(rule);
> +
>          return is_controller;
>      }
>      return false;
> @@ -4519,12 +4520,20 @@ rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout,
>      ofproto_rule_reduce_timeouts(&rule->up, idle_timeout, hard_timeout);
>  }
>
> -void
> -rule_dpif_get_actions(const struct rule_dpif *rule,
> -                      const struct ofpact **ofpacts, size_t *ofpacts_len)
> +/* Returns 'rule''s actions.  The caller owns a reference on the returned
> + * actions and must eventually release it (with rule_actions_unref()) to avoid
> + * a memory leak. */
> +struct rule_actions *
> +rule_dpif_get_actions(const struct rule_dpif *rule)
>  {
> -    *ofpacts = rule->up.ofpacts;
> -    *ofpacts_len = rule->up.ofpacts_len;
> +    struct rule_actions *actions;
> +
> +    ovs_rwlock_rdlock(&rule->up.rwlock);
> +    actions = rule->up.actions;
> +    rule_actions_ref(actions);
> +    ovs_rwlock_unlock(&rule->up.rwlock);
> +
> +    return actions;
>  }
>
>  /* Subfacets. */
> @@ -5308,7 +5317,8 @@ trace_format_rule(struct ds *result, int level, const struct rule_dpif *rule)
>
>      ds_put_char_multiple(result, '\t', level);
>      ds_put_cstr(result, "OpenFlow ");
> -    ofpacts_format(rule->up.ofpacts, rule->up.ofpacts_len, result);
> +    ofpacts_format(rule->up.actions->ofpacts, rule->up.actions->ofpacts_len,
> +                   result);
>      ds_put_char(result, '\n');
>  }
>
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index 7efc8d7..befd458 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -76,9 +76,7 @@ void rule_dpif_credit_stats(struct rule_dpif *rule ,
>
>  bool rule_dpif_fail_open(const struct rule_dpif *rule);
>
> -void rule_dpif_get_actions(const struct rule_dpif *rule,
> -                           const struct ofpact **ofpacts,
> -                           size_t *ofpacts_len);
> +struct rule_actions *rule_dpif_get_actions(const struct rule_dpif *);
>
>  ovs_be64 rule_dpif_get_flow_cookie(const struct rule_dpif *rule);
>
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 0b8a5e5..f8b856e 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -253,10 +253,8 @@ struct rule {
>      struct ovs_rwlock rwlock;
>
>      /* Guarded by rwlock. */
> -    struct ofpact *ofpacts;      /* Sequence of "struct ofpacts". */
> -    unsigned int ofpacts_len;    /* Size of 'ofpacts', in bytes. */
> +    struct rule_actions *actions;
>
> -    uint32_t meter_id;           /* Non-zero OF meter_id, or zero. */
>      struct list meter_list_node; /* In owning meter's 'rules' list. */
>
>      /* Flow monitors. */
> @@ -269,6 +267,30 @@ struct rule {
>                                   * is expirable, otherwise empty. */
>  };
>
> +/* A set of actions within a "struct rule".
> + *
> + *
> + * Thread-safety
> + * =============
> + *
> + * A struct rule_actions 'actions' may be accessed without a risk of being
> + * freed by code that holds a read-lock or write-lock on 'rule->rwlock' (where
> + * 'rule' is the rule for which 'rule->actions == actions') or that owns a
> + * reference to 'actions->ref_count' (or both). */
> +struct rule_actions {
> +    atomic_uint ref_count;
> +
> +    /* These members are immutable: they do not change during the struct's
> +     * lifetime.  */
> +    struct ofpact *ofpacts;     /* Sequence of "struct ofpacts". */
> +    unsigned int ofpacts_len;   /* Size of 'ofpacts', in bytes. */
> +    uint32_t meter_id;          /* Non-zero OF meter_id, or zero. */
> +};
> +
> +struct rule_actions *rule_actions_create(const struct ofpact *, size_t);
> +void rule_actions_ref(struct rule_actions *);
> +void rule_actions_unref(struct rule_actions *);
> +
>  /* Threshold at which to begin flow table eviction. Only affects the
>   * ofproto-dpif implementation */
>  extern unsigned flow_eviction_threshold;
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 458ff04..9399d41 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -124,9 +124,7 @@ struct ofoperation {
>
>      /* OFOPERATION_MODIFY, OFOPERATION_REPLACE: The old actions, if the actions
>       * are changing. */
> -    struct ofpact *ofpacts;
> -    size_t ofpacts_len;
> -    uint32_t meter_id;
> +    struct rule_actions *actions;
>
>      /* OFOPERATION_DELETE. */
>      enum ofp_flow_removed_reason reason; /* Reason flow was removed. */
> @@ -1741,20 +1739,29 @@ ofproto_add_flow(struct ofproto *ofproto, const struct match *match,
>                   const struct ofpact *ofpacts, size_t ofpacts_len)
>  {
>      const struct rule *rule;
> +    bool must_add;
>
>      /* First do a cheap check whether the rule we're looking for already exists
>       * with the actions that we want.  If it does, then we're done. */
>      ovs_rwlock_rdlock(&ofproto->tables[0].cls.rwlock);
>      rule = rule_from_cls_rule(classifier_find_match_exactly(
>                                    &ofproto->tables[0].cls, match, priority));
> +    if (rule) {
> +        ovs_rwlock_rdlock(&rule->rwlock);
> +        must_add = !ofpacts_equal(rule->actions->ofpacts,
> +                                  rule->actions->ofpacts_len,
> +                                  ofpacts, ofpacts_len);
> +        ovs_rwlock_unlock(&rule->rwlock);
> +    } else {
> +        must_add = true;
> +    }
>      ovs_rwlock_unlock(&ofproto->tables[0].cls.rwlock);
>
>      /* If there's no such rule or the rule doesn't have the actions we want,
>       * fall back to a executing a full flow mod.  We can't optimize this at
>       * all because we didn't take enough locks above to ensure that the flow
>       * table didn't already change beneath us.  */
> -    if (!rule || !ofpacts_equal(rule->ofpacts, rule->ofpacts_len,
> -                                ofpacts, ofpacts_len)) {
> +    if (must_add) {
>          simple_flow_mod(ofproto, match, priority, ofpacts, ofpacts_len,
>                          OFPFC_MODIFY_STRICT);
>      }
> @@ -2300,19 +2307,64 @@ static void
>  ofproto_rule_destroy__(struct rule *rule)
>  {
>      cls_rule_destroy(&rule->cr);
> -    free(rule->ofpacts);
> +    rule_actions_unref(rule->actions);
>      ovs_mutex_destroy(&rule->timeout_mutex);
>      ovs_rwlock_destroy(&rule->rwlock);
>      rule->ofproto->ofproto_class->rule_dealloc(rule);
>  }
>
> +/* Creates and returns a new 'struct rule_actions', with a ref_count of 1,
> + * whose actions are a copy of from the 'ofpacts_len' bytes of 'ofpacts'. */
> +struct rule_actions *
> +rule_actions_create(const struct ofpact *ofpacts, size_t ofpacts_len)
> +{
> +    struct rule_actions *actions;
> +
> +    actions = xmalloc(sizeof *actions);
> +    atomic_init(&actions->ref_count, 1);
> +    actions->ofpacts = xmemdup(ofpacts, ofpacts_len);
> +    actions->ofpacts_len = ofpacts_len;
> +    actions->meter_id = ofpacts_get_meter(ofpacts, ofpacts_len);
> +    return actions;
> +}
> +
> +/* Increments 'actions''s ref_count. */
> +void
> +rule_actions_ref(struct rule_actions *actions)
> +{
> +    if (actions) {
> +        unsigned int orig;
> +
> +        atomic_add(&actions->ref_count, 1, &orig);
> +        ovs_assert(orig != 0);
> +    }
> +}
> +
> +/* Decrements 'actions''s ref_count and frees 'actions' if the ref_count
> + * reaches 0. */
> +void
> +rule_actions_unref(struct rule_actions *actions)
> +{
> +    if (actions) {
> +        unsigned int orig;
> +
> +        atomic_sub(&actions->ref_count, 1, &orig);
> +        if (orig == 1) {
> +            free(actions);
> +        } else {
> +            ovs_assert(orig != 0);
> +        }
> +    }
> +}
> +
>  /* Returns true if 'rule' has an OpenFlow OFPAT_OUTPUT or OFPAT_ENQUEUE action
>   * that outputs to 'port' (output to OFPP_FLOOD and OFPP_ALL doesn't count). */
>  bool
>  ofproto_rule_has_out_port(const struct rule *rule, ofp_port_t port)
>  {
>      return (port == OFPP_ANY
> -            || ofpacts_output_to_port(rule->ofpacts, rule->ofpacts_len, port));
> +            || ofpacts_output_to_port(rule->actions->ofpacts,
> +                                      rule->actions->ofpacts_len, port));
>  }
>
>  /* Returns true if 'rule' has group and equals group_id. */
> @@ -2320,7 +2372,8 @@ bool
>  ofproto_rule_has_out_group(const struct rule *rule, uint32_t group_id)
>  {
>      return (group_id == OFPG11_ANY
> -            || ofpacts_output_to_group(rule->ofpacts, rule->ofpacts_len, group_id));
> +            || ofpacts_output_to_group(rule->actions->ofpacts,
> +                                       rule->actions->ofpacts_len, group_id));
>  }
>
>  /* Returns true if a rule related to 'op' has an OpenFlow OFPAT_OUTPUT or
> @@ -2339,7 +2392,8 @@ ofoperation_has_out_port(const struct ofoperation *op, ofp_port_t out_port)
>
>      case OFOPERATION_MODIFY:
>      case OFOPERATION_REPLACE:
> -        return ofpacts_output_to_port(op->ofpacts, op->ofpacts_len, out_port);
> +        return ofpacts_output_to_port(op->actions->ofpacts,
> +                                      op->actions->ofpacts_len, out_port);
>      }
>
>      NOT_REACHED();
> @@ -3124,8 +3178,8 @@ handle_flow_stats_request(struct ofconn *ofconn,
>          fs.hard_age = age_secs(now - rule->modified);
>          ofproto->ofproto_class->rule_get_stats(rule, &fs.packet_count,
>                                                 &fs.byte_count);
> -        fs.ofpacts = rule->ofpacts;
> -        fs.ofpacts_len = rule->ofpacts_len;
> +        fs.ofpacts = rule->actions->ofpacts;
> +        fs.ofpacts_len = rule->actions->ofpacts_len;
>
>          ovs_mutex_lock(&rule->timeout_mutex);
>          fs.idle_timeout = rule->idle_timeout;
> @@ -3163,7 +3217,8 @@ flow_stats_ds(struct rule *rule, struct ds *results)
>      ds_put_format(results, "n_bytes=%"PRIu64", ", byte_count);
>      cls_rule_format(&rule->cr, results);
>      ds_put_char(results, ',');
> -    ofpacts_format(rule->ofpacts, rule->ofpacts_len, results);
> +    ofpacts_format(rule->actions->ofpacts, rule->actions->ofpacts_len,
> +                   results);
>      ds_put_cstr(results, "\n");
>  }
>
> @@ -3553,9 +3608,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
>
>      rule->table_id = table - ofproto->tables;
>      rule->send_flow_removed = (fm->flags & OFPUTIL_FF_SEND_FLOW_REM) != 0;
> -    rule->ofpacts = xmemdup(fm->ofpacts, fm->ofpacts_len);
> -    rule->ofpacts_len = fm->ofpacts_len;
> -    rule->meter_id = ofpacts_get_meter(rule->ofpacts, rule->ofpacts_len);
> +    rule->actions = rule_actions_create(fm->ofpacts, fm->ofpacts_len);
>      list_init(&rule->meter_list_node);
>      rule->eviction_group = NULL;
>      list_init(&rule->expirable);
> @@ -3626,7 +3679,8 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
>          }
>
>          actions_changed = !ofpacts_equal(fm->ofpacts, fm->ofpacts_len,
> -                                         rule->ofpacts, rule->ofpacts_len);
> +                                         rule->actions->ofpacts,
> +                                         rule->actions->ofpacts_len);
>
>          op = ofoperation_create(group, rule, type, 0);
>
> @@ -3653,17 +3707,15 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
>
>          reset_counters = (fm->flags & OFPUTIL_FF_RESET_COUNTS) != 0;
>          if (actions_changed || reset_counters) {
> -            op->ofpacts = rule->ofpacts;
> -            op->ofpacts_len = rule->ofpacts_len;
> -            op->meter_id = rule->meter_id;
> +            struct rule_actions *new_actions;
> +
> +            op->actions = rule->actions;
> +            new_actions = rule_actions_create(fm->ofpacts, fm->ofpacts_len);
>
>              ovs_rwlock_wrlock(&rule->rwlock);
> -            rule->ofpacts = xmemdup(fm->ofpacts, fm->ofpacts_len);
> -            rule->ofpacts_len = fm->ofpacts_len;
> +            rule->actions = new_actions;
>              ovs_rwlock_unlock(&rule->rwlock);
>
> -            rule->meter_id = ofpacts_get_meter(rule->ofpacts,
> -                                               rule->ofpacts_len);
>              rule->ofproto->ofproto_class->rule_modify_actions(rule,
>                                                                reset_counters);
>          } else {
> @@ -4168,6 +4220,7 @@ ofproto_compose_flow_refresh_update(const struct rule *rule,
>                                      struct list *msgs)
>  {
>      struct ofoperation *op = rule->pending;
> +    const struct rule_actions *actions;
>      struct ofputil_flow_update fu;
>      struct match match;
>
> @@ -4189,12 +4242,11 @@ ofproto_compose_flow_refresh_update(const struct rule *rule,
>      minimatch_expand(&rule->cr.match, &match);
>      fu.match = &match;
>      fu.priority = rule->cr.priority;
> +
>      if (!(flags & NXFMF_ACTIONS)) {
> -        fu.ofpacts = NULL;
> -        fu.ofpacts_len = 0;
> +        actions = NULL;
>      } else if (!op) {
> -        fu.ofpacts = rule->ofpacts;
> -        fu.ofpacts_len = rule->ofpacts_len;
> +        actions = rule->actions;
>      } else {
>          /* An operation is in progress.  Use the previous version of the flow's
>           * actions, so that when the operation commits we report the change. */
> @@ -4204,24 +4256,19 @@ ofproto_compose_flow_refresh_update(const struct rule *rule,
>
>          case OFOPERATION_MODIFY:
>          case OFOPERATION_REPLACE:
> -            if (op->ofpacts) {
> -                fu.ofpacts = op->ofpacts;
> -                fu.ofpacts_len = op->ofpacts_len;
> -            } else {
> -                fu.ofpacts = rule->ofpacts;
> -                fu.ofpacts_len = rule->ofpacts_len;
> -            }
> +            actions = op->actions ? op->actions : rule->actions;
>              break;
>
>          case OFOPERATION_DELETE:
> -            fu.ofpacts = rule->ofpacts;
> -            fu.ofpacts_len = rule->ofpacts_len;
> +            actions = rule->actions;
>              break;
>
>          default:
>              NOT_REACHED();
>          }
>      }
> +    fu.ofpacts = actions ? actions->ofpacts : NULL;
> +    fu.ofpacts_len = actions ? actions->ofpacts_len : 0;
>
>      if (list_is_empty(msgs)) {
>          ofputil_start_flow_update(msgs);
> @@ -5444,7 +5491,7 @@ ofopgroup_complete(struct ofopgroup *group)
>          if (!(op->error
>                || ofproto_rule_is_hidden(rule)
>                || (op->type == OFOPERATION_MODIFY
> -                  && op->ofpacts
> +                  && op->actions
>                    && rule->flow_cookie == op->flow_cookie))) {
>              /* Check that we can just cast from ofoperation_type to
>               * nx_flow_update_event. */
> @@ -5519,16 +5566,16 @@ ofopgroup_complete(struct ofopgroup *group)
>                  rule->idle_timeout = op->idle_timeout;
>                  rule->hard_timeout = op->hard_timeout;
>                  ovs_mutex_unlock(&rule->timeout_mutex);
> -                if (op->ofpacts) {
> -                    free(rule->ofpacts);
> +                if (op->actions) {
> +                    struct rule_actions *old_actions;
>
>                      ovs_rwlock_wrlock(&rule->rwlock);
> -                    rule->ofpacts = op->ofpacts;
> -                    rule->ofpacts_len = op->ofpacts_len;
> +                    old_actions = rule->actions;
> +                    rule->actions = op->actions;
>                      ovs_rwlock_unlock(&rule->rwlock);
>
> -                    op->ofpacts = NULL;
> -                    op->ofpacts_len = 0;
> +                    op->actions = NULL;
> +                    rule_actions_unref(old_actions);
>                  }
>                  rule->send_flow_removed = op->send_flow_removed;
>              }
> @@ -5612,7 +5659,7 @@ ofoperation_destroy(struct ofoperation *op)
>          hmap_remove(&group->ofproto->deletions, &op->hmap_node);
>      }
>      list_remove(&op->group_node);
> -    free(op->ofpacts);
> +    rule_actions_unref(op->actions);
>      free(op);
>  }
>
> @@ -6110,8 +6157,9 @@ oftable_insert_rule(struct rule *rule)
>          ovs_mutex_unlock(&ofproto->expirable_mutex);
>      }
>      cookies_insert(ofproto, rule);
> -    if (rule->meter_id) {
> -        struct meter *meter = ofproto->meters[rule->meter_id];
> +
> +    if (rule->actions->meter_id) {
> +        struct meter *meter = ofproto->meters[rule->actions->meter_id];
>          list_insert(&meter->rules, &rule->meter_list_node);
>      }
>      ovs_rwlock_wrlock(&table->cls.rwlock);
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list