[ovs-dev] [dpif-netdev 14/15] dpif-netdev: Make thread-safety much more granular.

Ben Pfaff blp at nicira.com
Wed Jan 8 23:57:55 UTC 2014


On Wed, Jan 08, 2014 at 03:32:25PM -0800, Pravin Shelar wrote:
> On Fri, Dec 27, 2013 at 8:03 PM, Ben Pfaff <blp at nicira.com> wrote:
> > This will allow for parallelism in multithreaded forwarding in an upcoming
> > commit.
> >
> > Signed-off-by: Ben Pfaff <blp at nicira.com>

...

> > +static struct dp_netdev_flow *
> > +dp_netdev_flow_ref(const struct dp_netdev_flow *flow_)
> > +{
> > +    struct dp_netdev_flow *flow = CONST_CAST(struct dp_netdev_flow *, flow_);
> > +    if (flow) {
> > +        ovs_refcount_ref(&flow->ref_cnt);
> > +    }
> > +    return flow;
> > +}
> > +
> Can we have lock annotations for taking ref on action?

I think that we could, but I don't think it would be very useful
because these references aren't lexically balanced in one of the three
callers.  That is, dpif_netdev_flow_dump_next() returns holding a
reference, intentionally, which only next the next call to that
function or to dpif_netdev_flow_dump_done() will release.

> > +static void
> > +dp_netdev_flow_unref(struct dp_netdev_flow *flow)
> > +{
> > +    if (flow && ovs_refcount_unref(&flow->ref_cnt) == 1) {
> > +        cls_rule_destroy(CONST_CAST(struct cls_rule *, &flow->cr));
> > +        ovs_mutex_lock(&flow->mutex);
> > +        dp_netdev_actions_unref(flow->actions);
> > +        ovs_mutex_unlock(&flow->mutex);
> 
> do we really need to take lock here ?

No.  Clang complains if we don't, because of the lock annotations:

    ../lib/dpif-netdev.c:851:39: error: reading variable 'actions' requires locking any mutex [-Werror,-Wthread-safety-analysis]
            dp_netdev_actions_unref(flow->actions);

At some point we probably will want to introduce some "fake locking"
functions that are no-ops but satisfy Clang, but we haven't done that
yet and our current practice is to just take the locks anyway.

> > @@ -931,20 +1101,31 @@ dpif_netdev_flow_get(const struct dpif *dpif,
> >          return error;
> >      }
> >
> > -    ovs_mutex_lock(&dp_netdev_mutex);
> > +    ovs_rwlock_rdlock(&dp->cls.rwlock);
> >      netdev_flow = dp_netdev_find_flow(dp, &key);
> > +    ovs_rwlock_unlock(&dp->cls.rwlock);
> > +
> 
> This needs dp->flow_mutex, since it does lookup flow_table. or
> hmap_insert locking needs fix.

Thanks.

The comments show that I intended for dp->cls.rwlock to protect both
dp->cls and dp->flow_table, so in dp_netdev_flow_add() I moved the
hmap_insert() inside the existing block that holds dp->cls.rwlock.

> It will be less confusing if classifier and flow table lookup
> functions are named explicitly.

Do you have a suggestion?  I have spent too much time looking at this
code, myself.

> > +    ovs_mutex_init(&netdev_flow->mutex);
> > +    ovs_mutex_lock(&netdev_flow->mutex);
> > +
> >      netdev_flow->actions = dp_netdev_actions_create(actions, actions_len);
> >
> >      match_init(&match, flow, wc);
> > -    cls_rule_init(&netdev_flow->cr, &match, NETDEV_RULE_PRIORITY);
> > +    cls_rule_init(CONST_CAST(struct cls_rule *, &netdev_flow->cr),
> > +                  &match, NETDEV_RULE_PRIORITY);
> >      ovs_rwlock_wrlock(&dp->cls.rwlock);
> > -    classifier_insert(&dp->cls, &netdev_flow->cr);
> > +    classifier_insert(&dp->cls,
> > +                      CONST_CAST(struct cls_rule *, &netdev_flow->cr));
> >      ovs_rwlock_unlock(&dp->cls.rwlock);
> >
> > -    hmap_insert(&dp->flow_table, &netdev_flow->node, flow_hash(flow, 0));
> > +    ovs_mutex_unlock(&netdev_flow->mutex);
> > +
> flow->mutex lock can be released once actions pointer is set, right?
> or does it protect something more?
> At this point no one can have access to this flow, so this locking
> must be for sanity checker.

Yes, all it us protects us against, at this point, is Clang warnings.
Again, I guess we should work on that, and I'm sure we will at some
point.

> > +
> > +exit:
> > +    ovs_rwlock_unlock(&dp->port_rwlock);
> > +    dp_netdev_unref(dp);
> >  }
> >
> >  static void
> 
> How much does separate flow_mutex lock helps over just cls.rwlock?

I have not measured the advantage, but the idea is that cls.rwlock
should be held for writing as briefly as possible, because it delays
packet forwarding.  However, we need some way to ensure that writers
(and there might be several threads trying to write) can prevent the
flow table from changing as they check for some property and then act
based on it (e.g. add a flow only if one doesn't already exist), so
flow_mutex allows for that without delaying forwarding threads.



More information about the dev mailing list