[ovs-dev] [PATCH 2/3] ofproto: Make taking rule reference conditional on lookup.

Ethan Jackson ethan at nicira.com
Wed Apr 23 23:53:58 UTC 2014


Traditionally we've put function comments just in the .c file, not in
the .c and .h file as you've done for rule_dpif_lookup().  That said,
I don't know where that convention came from and don't care that much
. . .

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


On Wed, Apr 23, 2014 at 4:20 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> Prior to this paths the rule lookup functions have always taken a
> reference on the found rule before returning.  Make this conditional,
> so that unnecessary refs/unrefs can be avoided in a later patch.
>
> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> ---
>  ofproto/ofproto-dpif-xlate.c |    8 +++---
>  ofproto/ofproto-dpif.c       |   58 +++++++++++++++++++++++++++++-------------
>  ofproto/ofproto-dpif.h       |   20 ++++++++++++---
>  3 files changed, 62 insertions(+), 24 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 248382f..c62424a 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2057,7 +2057,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
>                                                !skip_wildcards
>                                                ? &ctx->xout->wc : NULL,
>                                                honor_table_miss,
> -                                              &ctx->table_id, &rule);
> +                                              &ctx->table_id, &rule, true);
>          ctx->xin->flow.in_port.ofp_port = old_in_port;
>
>          if (ctx->xin->resubmit_hook) {
> @@ -2090,7 +2090,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
>          }
>
>          choose_miss_rule(config, ctx->xbridge->miss_rule,
> -                         ctx->xbridge->no_packet_in_rule, &rule);
> +                         ctx->xbridge->no_packet_in_rule, &rule, true);
>
>  match:
>          if (rule) {
> @@ -2654,7 +2654,7 @@ xlate_learn_action(struct xlate_ctx *ctx,
>          entry = xlate_cache_add_entry(ctx->xin->xcache, XC_LEARN);
>          entry->u.learn.ofproto = ctx->xin->ofproto;
>          rule_dpif_lookup(ctx->xbridge->ofproto, &ctx->xin->flow, NULL,
> -                         &entry->u.learn.rule);
> +                         &entry->u.learn.rule, true);
>      }
>  }
>
> @@ -3263,7 +3263,7 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out *xout)
>      if (!xin->ofpacts && !ctx.rule) {
>          ctx.table_id = rule_dpif_lookup(ctx.xbridge->ofproto, flow,
>                                          !xin->skip_wildcards ? wc : NULL,
> -                                        &rule);
> +                                        &rule, true);
>          if (ctx.xin->resubmit_stats) {
>              rule_dpif_credit_stats(rule, ctx.xin->resubmit_stats);
>          }
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 983cad5..5669cd1 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -3189,10 +3189,16 @@ rule_dpif_get_actions(const struct rule_dpif *rule)
>   *
>   * The return value will be zero unless there was a miss and
>   * OFPTC11_TABLE_MISS_CONTINUE is in effect for the sequence of tables
> - * where misses occur. */
> + * where misses occur.
> + *
> + * The rule is returned in '*rule', which is valid at least until the next
> + * RCU quiescent period.  If the '*rule' needs to stay around longer,
> + * a non-zero 'take_ref' must be passed in to cause a reference to be taken
> + * on it before this returns. */
>  uint8_t
>  rule_dpif_lookup(struct ofproto_dpif *ofproto, struct flow *flow,
> -                 struct flow_wildcards *wc, struct rule_dpif **rule)
> +                 struct flow_wildcards *wc, struct rule_dpif **rule,
> +                 bool take_ref)
>  {
>      enum rule_dpif_lookup_verdict verdict;
>      enum ofputil_port_config config = 0;
> @@ -3219,7 +3225,7 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, struct flow *flow,
>      }
>
>      verdict = rule_dpif_lookup_from_table(ofproto, flow, wc, true,
> -                                          &table_id, rule);
> +                                          &table_id, rule, take_ref);
>
>      switch (verdict) {
>      case RULE_DPIF_LOOKUP_VERDICT_MATCH:
> @@ -3248,13 +3254,17 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, struct flow *flow,
>      }
>
>      choose_miss_rule(config, ofproto->miss_rule,
> -                     ofproto->no_packet_in_rule, rule);
> +                     ofproto->no_packet_in_rule, rule, take_ref);
>      return table_id;
>  }
>
> +/* The returned rule is valid at least until the next RCU quiescent period.
> + * If the '*rule' needs to stay around longer, a non-zero 'take_ref' must be
> + * passed in to cause a reference to be taken on it before this returns. */
>  static struct rule_dpif *
>  rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, uint8_t table_id,
> -                          const struct flow *flow, struct flow_wildcards *wc)
> +                          const struct flow *flow, struct flow_wildcards *wc,
> +                          bool take_ref)
>  {
>      struct classifier *cls = &ofproto->up.tables[table_id].cls;
>      const struct cls_rule *cls_rule;
> @@ -3288,7 +3298,9 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, uint8_t table_id,
>      }
>
>      rule = rule_dpif_cast(rule_from_cls_rule(cls_rule));
> -    rule_dpif_ref(rule);
> +    if (take_ref) {
> +        rule_dpif_ref(rule);
> +    }
>      fat_rwlock_unlock(&cls->rwlock);
>
>      return rule;
> @@ -3323,13 +3335,19 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, uint8_t table_id,
>   *
>   *    - RULE_DPIF_LOOKUP_VERDICT_DEFAULT if no rule was found,
>   *      'honor_table_miss' is true and a table miss configuration has
> - *      not been specified in this case. */
> + *      not been specified in this case.
> + *
> + * The rule is returned in '*rule', which is valid at least until the next
> + * RCU quiescent period.  If the '*rule' needs to stay around longer,
> + * a non-zero 'take_ref' must be passed in to cause a reference to be taken
> + * on it before this returns. */
>  enum rule_dpif_lookup_verdict
>  rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
>                              const struct flow *flow,
>                              struct flow_wildcards *wc,
>                              bool honor_table_miss,
> -                            uint8_t *table_id, struct rule_dpif **rule)
> +                            uint8_t *table_id, struct rule_dpif **rule,
> +                            bool take_ref)
>  {
>      uint8_t next_id;
>
> @@ -3338,7 +3356,8 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
>           next_id++, next_id += (next_id == TBL_INTERNAL))
>      {
>          *table_id = next_id;
> -        *rule = rule_dpif_lookup_in_table(ofproto, *table_id, flow, wc);
> +        *rule = rule_dpif_lookup_in_table(ofproto, *table_id, flow, wc,
> +                                          take_ref);
>          if (*rule) {
>              return RULE_DPIF_LOOKUP_VERDICT_MATCH;
>          } else if (!honor_table_miss) {
> @@ -3365,13 +3384,21 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
>
>  /* Given a port configuration (specified as zero if there's no port), chooses
>   * which of 'miss_rule' and 'no_packet_in_rule' should be used in case of a
> - * flow table miss. */
> + * flow table miss.
> + *
> + * The rule is returned in '*rule', which is valid at least until the next
> + * RCU quiescent period.  If the '*rule' needs to stay around longer,
> + * a reference must be taken on it (rule_dpif_ref()).
> + */
>  void
>  choose_miss_rule(enum ofputil_port_config config, struct rule_dpif *miss_rule,
> -                 struct rule_dpif *no_packet_in_rule, struct rule_dpif **rule)
> +                 struct rule_dpif *no_packet_in_rule, struct rule_dpif **rule,
> +                 bool take_ref)
>  {
>      *rule = config & OFPUTIL_PC_NO_PACKET_IN ? no_packet_in_rule : miss_rule;
> -    rule_dpif_ref(*rule);
> +    if (take_ref) {
> +        rule_dpif_ref(*rule);
> +    }
>  }
>
>  void
> @@ -4197,7 +4224,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
>      if (ofpacts) {
>          rule = NULL;
>      } else {
> -        rule_dpif_lookup(ofproto, flow, &trace.wc, &rule);
> +        rule_dpif_lookup(ofproto, flow, &trace.wc, &rule, false);
>
>          trace_format_rule(ds, 0, rule);
>          if (rule == ofproto->miss_rule) {
> @@ -4253,8 +4280,6 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
>
>          xlate_out_uninit(&trace.xout);
>      }
> -
> -    rule_dpif_unref(rule);
>  }
>
>  /* Store the current ofprotos in 'ofproto_shash'.  Returns a sorted list
> @@ -4819,9 +4844,8 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto,
>      }
>
>      rule = rule_dpif_lookup_in_table(ofproto, TBL_INTERNAL, &match->flow,
> -                                     &match->wc);
> +                                     &match->wc, false);
>      if (rule) {
> -        rule_dpif_unref(rule);
>          *rulep = &rule->up;
>      } else {
>          OVS_NOT_REACHED();
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index eb4787c..88b6684 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -86,15 +86,25 @@ extern struct ovs_rwlock xlate_rwlock;
>  size_t ofproto_dpif_get_max_mpls_depth(const struct ofproto_dpif *);
>  bool ofproto_dpif_get_enable_recirc(const struct ofproto_dpif *);
>
> +/* The rule is returned in '*rule', which is valid at least until the next
> + * RCU quiescent period.  If the '*rule' needs to stay around longer,
> + * a non-zero 'take_ref' must be passed in to cause a reference to be taken
> + * on it before this returns. */
>  uint8_t rule_dpif_lookup(struct ofproto_dpif *, struct flow *,
> -                      struct flow_wildcards *, struct rule_dpif **rule);
> +                         struct flow_wildcards *, struct rule_dpif **rule,
> +                         bool take_ref);
>
> +/* The rule is returned in '*rule', which is valid at least until the next
> + * RCU quiescent period.  If the '*rule' needs to stay around longer,
> + * a non-zero 'take_ref' must be passed in to cause a reference to be taken
> + * on it before this returns. */
>  enum rule_dpif_lookup_verdict rule_dpif_lookup_from_table(struct ofproto_dpif *,
>                                                            const struct flow *,
>                                                            struct flow_wildcards *,
>                                                            bool force_controller_on_miss,
>                                                            uint8_t *table_id,
> -                                                          struct rule_dpif **rule);
> +                                                          struct rule_dpif **rule,
> +                                                          bool take_ref);
>
>  void rule_dpif_ref(struct rule_dpif *);
>  void rule_dpif_unref(struct rule_dpif *);
> @@ -114,10 +124,14 @@ ovs_be64 rule_dpif_get_flow_cookie(const struct rule_dpif *rule);
>  void rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout,
>                                 uint16_t hard_timeout);
>
> +/* The rule is returned in '*rule', which is valid at least until the next
> + * RCU quiescent period.  If the '*rule' needs to stay around longer,
> + * a non-zero 'take_ref' must be passed in to cause a reference to be taken
> + * on it before this returns. */
>  void choose_miss_rule(enum ofputil_port_config,
>                        struct rule_dpif *miss_rule,
>                        struct rule_dpif *no_packet_in_rule,
> -                      struct rule_dpif **rule);
> +                      struct rule_dpif **rule, bool take_ref);
>
>  bool group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id,
>                         struct group_dpif **group);
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list