[ovs-dev] [PATCH] ofproto-dpif: Use a regular ref instead of try_ref for rule translation.

Ben Pfaff blp at nicira.com
Mon Aug 3 23:46:00 UTC 2015


Thanks Jarno, I applied this to master.

On Mon, Aug 03, 2015 at 02:31:29PM -0700, Jarno Rajahalme wrote:
> Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
> 
> > On Aug 2, 2015, at 11:51 AM, Ben Pfaff <blp at nicira.com> wrote:
> > 
> > Until now, flow translation has had to use try_ref to take a reference on
> > a rule, because a competing thread might have released the last reference
> > and done an RCU-postponed deletion.  Since classifier versioning was
> > introduced, however, the release of the last reference is itself
> > RCU-postponed, which means that it is always safe to take the reference
> > directly.
> > 
> > Changing try_ref to ref means that taking a reference can't fail, which
> > allows the caller to take a reference in cases where the need to take a
> > reference was previously passed along a call chain, which simplifies some
> > code.
> > 
> > CC: Jarno Rajahalme <jrajahalme at nicira.com>
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> > ofproto/ofproto-dpif-xlate.c |  5 +++--
> > ofproto/ofproto-dpif.c       | 38 ++++++++++----------------------------
> > ofproto/ofproto-dpif.h       | 10 ----------
> > 3 files changed, 13 insertions(+), 40 deletions(-)
> > 
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index e2596d9..a34b5f9 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -3133,7 +3133,6 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
> >         rule = rule_dpif_lookup_from_table(ctx->xbridge->ofproto,
> >                                            ctx->tables_version,
> >                                            &ctx->xin->flow, ctx->xin->wc,
> > -                                           ctx->xin->xcache != NULL,
> >                                            ctx->xin->resubmit_stats,
> >                                            &ctx->table_id, in_port,
> >                                            may_packet_in, honor_table_miss);
> > @@ -3152,6 +3151,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
> > 
> >                 entry = xlate_cache_add_entry(ctx->xin->xcache, XC_RULE);
> >                 entry->u.rule = rule;
> > +                rule_dpif_ref(rule);
> >             }
> >             xlate_recursively(ctx, rule);
> >         }
> > @@ -4912,7 +4912,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
> >     if (!xin->ofpacts && !ctx.rule) {
> >         ctx.rule = rule_dpif_lookup_from_table(
> >             ctx.xbridge->ofproto, ctx.tables_version, flow, xin->wc,
> > -            ctx.xin->xcache != NULL, ctx.xin->resubmit_stats, &ctx.table_id,
> > +            ctx.xin->resubmit_stats, &ctx.table_id,
> >             flow->in_port.ofp_port, true, true);
> >         if (ctx.xin->resubmit_stats) {
> >             rule_dpif_credit_stats(ctx.rule, ctx.xin->resubmit_stats);
> > @@ -4922,6 +4922,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
> > 
> >             entry = xlate_cache_add_entry(ctx.xin->xcache, XC_RULE);
> >             entry->u.rule = ctx.rule;
> > +            rule_dpif_ref(ctx.rule);
> >         }
> > 
> >         if (OVS_UNLIKELY(ctx.xin->resubmit_hook)) {
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 70faa13..30db4fe 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -3765,29 +3765,19 @@ ofproto_dpif_get_tables_version(struct ofproto_dpif *ofproto OVS_UNUSED)
> > }
> > 
> > /* The returned rule (if any) 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.
> > + * period.  If the rule needs to stay around longer, the caller should take
> > + * a reference.
> >  *
> >  * 'flow' is non-const to allow for temporary modifications during the lookup.
> >  * Any changes are restored before returning. */
> > static struct rule_dpif *
> > rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, cls_version_t version,
> >                           uint8_t table_id, struct flow *flow,
> > -                          struct flow_wildcards *wc, bool take_ref)
> > +                          struct flow_wildcards *wc)
> > {
> >     struct classifier *cls = &ofproto->up.tables[table_id].cls;
> > -    const struct cls_rule *cls_rule;
> > -    struct rule_dpif *rule;
> > -
> > -    do {
> > -        cls_rule = classifier_lookup(cls, version, flow, wc);
> > -
> > -        rule = rule_dpif_cast(rule_from_cls_rule(cls_rule));
> > -
> > -        /* Try again if the rule was released before we get the reference. */
> > -    } while (rule && take_ref && !rule_dpif_try_ref(rule));
> > -
> > -    return rule;
> > +    return rule_dpif_cast(rule_from_cls_rule(classifier_lookup(cls, version,
> > +                                                               flow, wc)));
> > }
> > 
> > /* Look up 'flow' in 'ofproto''s classifier version 'version', starting from
> > @@ -3807,9 +3797,8 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, cls_version_t version,
> >  * '*table_id'.
> >  *
> >  * 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.
> > + * RCU quiescent period.  If the '*rule' needs to stay around longer, the
> > + * caller must take a reference.
> >  *
> >  * 'in_port' allows the lookup to take place as if the in port had the value
> >  * 'in_port'.  This is needed for resubmit action support.
> > @@ -3819,7 +3808,7 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, cls_version_t version,
> > struct rule_dpif *
> > rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
> >                             cls_version_t version, struct flow *flow,
> > -                            struct flow_wildcards *wc, bool take_ref,
> > +                            struct flow_wildcards *wc,
> >                             const struct dpif_flow_stats *stats,
> >                             uint8_t *table_id, ofp_port_t in_port,
> >                             bool may_packet_in, bool honor_table_miss)
> > @@ -3842,9 +3831,6 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
> >             /* Must be OFPC_FRAG_DROP (we don't have OFPC_FRAG_REASM).
> >              * Use the drop_frags_rule (which cannot disappear). */
> >             rule = ofproto->drop_frags_rule;
> > -            if (take_ref) {
> > -                rule_dpif_ref(rule);
> > -            }
> >             if (stats) {
> >                 struct oftable *tbl = &ofproto->up.tables[*table_id];
> >                 unsigned long orig;
> > @@ -3871,8 +3857,7 @@ 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, version, next_id, flow, wc,
> > -                                         take_ref);
> > +        rule = rule_dpif_lookup_in_table(ofproto, version, next_id, flow, wc);
> >         if (stats) {
> >             struct oftable *tbl = &ofproto->up.tables[next_id];
> >             unsigned long orig;
> > @@ -3911,9 +3896,6 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
> >             rule = ofproto->miss_rule;
> >         }
> >     }
> > -    if (take_ref) {
> > -        rule_dpif_ref(rule);
> > -    }
> > out:
> >     /* Restore port numbers, as they may have been modified above. */
> >     flow->tp_src = old_tp_src;
> > @@ -5520,7 +5502,7 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto,
> >     rule = rule_dpif_lookup_in_table(ofproto,
> >                                      ofproto_dpif_get_tables_version(ofproto),
> >                                      TBL_INTERNAL, &ofm.fm.match.flow,
> > -                                     &ofm.fm.match.wc, false);
> > +                                     &ofm.fm.match.wc);
> >     if (rule) {
> >         *rulep = &rule->up;
> >     } else {
> > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> > index 0209565..69ca54c 100644
> > --- a/ofproto/ofproto-dpif.h
> > +++ b/ofproto/ofproto-dpif.h
> > @@ -102,7 +102,6 @@ cls_version_t ofproto_dpif_get_tables_version(struct ofproto_dpif *);
> > struct rule_dpif *rule_dpif_lookup_from_table(struct ofproto_dpif *,
> >                                               cls_version_t, struct flow *,
> >                                               struct flow_wildcards *,
> > -                                              bool take_ref,
> >                                               const struct dpif_flow_stats *,
> >                                               uint8_t *table_id,
> >                                               ofp_port_t in_port,
> > @@ -200,15 +199,6 @@ static inline void rule_dpif_ref(struct rule_dpif *rule)
> >     }
> > }
> > 
> > -static inline bool rule_dpif_try_ref(struct rule_dpif *rule)
> > -{
> > -    if (rule) {
> > -        return ofproto_rule_try_ref(RULE_CAST(rule));
> > -    }
> > -    return false;
> > -}
> > -
> > -
> > static inline void rule_dpif_unref(struct rule_dpif *rule)
> > {
> >     if (rule) {
> > -- 
> > 2.1.3
> > 
> 



More information about the dev mailing list