[ovs-dev] [PATCH v2 5/5] ofproto-dpif: Use ovs_refcount_try_ref_rcu().

[Jarno Rajahalme]

This small change made the last patch of the original series not apply any more, I’ll send a rebased v3 series out in a moment to keep things simple. No other changes, so if reviews are underway, they should apply as-is.

  Jarno

On Jul 7, 2014, at 1:40 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:

> Thanks for the reviews, series pushed with suggested changes (upto this patch),
> 
>  Jarno
> 
> On Jul 7, 2014, at 9:47 AM, Ben Pfaff <blp at nicira.com> wrote:
> 
>> On Fri, Jul 04, 2014 at 07:21:19AM -0700, Jarno Rajahalme wrote:
>>> This is a prerequisite step in making the classifier lookups lockless.
>>> If taking a reference fails, we do the lookup again, as a new (lower
>>> priority) rule may now match instead.
>>> 
>>> Also remove unwildcarding dl_type and nw_frag, as these are already
>>> taken care of by xlate_actions().
>>> 
>>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
>> 
>> I'd be inclined to write the retry loop in rule_dpif_lookup_in_table()
>> as an actual "do-while" loop.
> 
> Yeah, definitely cleaner this way, thanks for the hint:
> 
> @@ -3383,20 +3383,15 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, uint8_t table_id,
>         }
>     }
> 
> -retry:
> -    fat_rwlock_rdlock(&cls->rwlock);
> -    cls_rule = classifier_lookup(cls, flow, wc);
> -    fat_rwlock_unlock(&cls->rwlock);
> -
> -    rule = rule_dpif_cast(rule_from_cls_rule(cls_rule));
> -    if (rule && take_ref) {
> -        if (!rule_dpif_try_ref(rule)) {
> -            /* The rule was released before we got the ref, so it
> -             * cannot be in the classifier any more.  Do another
> -             * lookup to find another rule, if any. */
> -            goto retry;
> -        }
> -    }
> +    do {
> +        fat_rwlock_rdlock(&cls->rwlock);
> +        cls_rule = classifier_lookup(cls, flow, wc);
> +        fat_rwlock_unlock(&cls->rwlock);
> +
> +        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;
> }
> 
> 
>> 
>> Acked-by: Ben Pfaff <blp at nicira.com>
>