On Wed, Aug 07, 2013 at 05:31:57PM -0700, Andy Zhou wrote:
> On Thu, Aug 1, 2013 at 2:29 PM, Ben Pfaff <b...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
