[ovs-dev] [netdev 27/27] netdev: Make netdev access thread-safe.

[Ben Pfaff]

On Wed, Aug 07, 2013 at 05:31:57PM -0700, Andy Zhou wrote:
> On Thu, Aug 1, 2013 at 2:29 PM, Ben Pfaff <blp at nicira.com> wrote:
> > diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
> >  netdev_dummy_get_ifindex(const struct netdev *netdev)
> >  {
> >      struct netdev_dummy *dev = netdev_dummy_cast(netdev);
> > +    int ifindex;
> > +
> > +    ovs_mutex_lock(&dev->mutex);
> > +    ifindex = dev->ifindex;
> > +    ovs_mutex_unlock(&dev->mutex);
> >
> > -    return dev->ifindex;
> > +    return ifindex;
> >  }
> >
> Is mutex lock necessary?  same for netdev_dummy_get_mtu().

In the real world, I doubt that the mutex lock is necessary here.  But
it's not a fast path so I see no reason to try to optimize it.

> >  static unsigned int
> > -netdev_dummy_change_seq(const struct netdev *netdev)
> > +netdev_dummy_change_seq(const struct netdev *netdev_)
> >  {
> > -    return netdev_dummy_cast(netdev)->change_seq;
> > +    struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
> > +    unsigned int change_seq;
> > +
> > +    ovs_mutex_lock(&netdev->mutex);
> > +    change_seq = netdev->change_seq;
> > +    ovs_mutex_unlock(&netdev->mutex);
> > +
> > +    return change_seq;
> >  }
> >
> > Same here. Not sure mutex lock is necessary.

Same answer ;-)

> > @@ -1874,15 +1927,17 @@ netdev_linux_get_qos(const struct netdev *netdev_,
> >      struct netdev_linux *netdev = netdev_linux_cast(netdev_);
> >      int error;
> >
> > +    ovs_mutex_lock(&netdev->mutex);
> >      error = tc_query_qdisc(netdev_);
> > -    if (error) {
> > -        return error;
> > +    if (!error) {
> > +        *typep = netdev->tc->ops->ovs_name;
> > +        error = (netdev->tc->ops->qdisc_get
> > +                 ? netdev->tc->ops->qdisc_get(netdev_, details)
> > +                 : 0);
> >      }
> > +    ovs_mutex_unlock(&netdev->mutex);
> >
> How to reason that acccess to *typep outside after mutex unlock will be
> safe?

The values currently used are, I believe, all constant string literals,
so they are safe.  Maybe we will introduce some other non-constant
strings later; I agree that to make this safe requires an API change.

> > +static void
> > +netdev_class_foreach(void (*cb)(const struct netdev_class *))
> > +{
> > +    struct netdev_registered_class *rc_to_unref = NULL;
> > +    struct netdev_registered_class *rc;
> > +
> > +    ovs_mutex_lock(&netdev_mutex);
> > +    HMAP_FOR_EACH (rc, hmap_node, &netdev_classes) {
> > +        netdev_registered_class_unref(rc_to_unref);
> > +        rc_to_unref = rc;
> > +        rc->ref_cnt++;
> > +
> > +        ovs_mutex_unlock(&netdev_mutex);
> >
> How to reason this unlock is safe?

This function takes a reference on 'rc', so we can be sure that the
class won't be freed while the mutex is unlocked.

> > +        cb(rc->class);
> > +        ovs_mutex_lock(&netdev_mutex);
> > +    }
> > +    netdev_registered_class_unref(rc_to_unref);
> > +    ovs_mutex_unlock(&netdev_mutex);
> > +}

Thanks,

Ben.