[ovs-dev] [netlink v4 47/52] datapath: Adopt Generic Netlink-compatible locking.

Jesse Gross jesse at nicira.com
Wed Jan 26 17:15:35 UTC 2011


On Tue, Jan 25, 2011 at 9:30 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Tue, Jan 25, 2011 at 04:48:08PM -0800, Jesse Gross wrote:
>> On Tue, Jan 25, 2011 at 11:20 AM, Ben Pfaff <blp at nicira.com> wrote:
>> DPs somewhat straddle the line as far as being protected by genl vs
>> RTNL.  We could just document that it is safe to access it with either
>> and both are required to modify but I'm tempted to just enforce
>> RTNL/RCU.  There are only a few places that don't do this already, so
>> we could just take RCU in these cases.
>
> I guess I don't see the benefit to this change.  RTNL is required to
> create and delete datapaths only because those operations create and
> destroy network devices as side effects.  RTNL isn't needed for anything
> else related to datapaths (and I see now that I was overzealous about
> taking RTNL in set_datapath(); that isn't needed).

I would actually much prefer that genl_lock is used to protect DPs (I
agree that the fact that it takes RTNL in the add/destroy cases isn't
that important because it's not protecting the dp data structure
itself).

The primary concern that I have is with the things in dp_notify.c
because they only have RTNL and make modifications.  The worst
offender is NETDEV_UNREGISTER, which unfortunately is the most useful.
 First, dp_detach_port() is documented as requiring genl_lock and RTNL
but only RTNL is held here.  I believe that we currently access all of
the fields that it modifies while holding either RTNL or RCU but that
conflicts with DP being protected by genl_lock.  One consolation is
that it looks like dp->n_ports is write-only now so we can drop it and
avoid a problem there.

The other notifications in dp_device_event() are at least read-only
from the perspective of the DP, so we could in theory hold
rcu_read_lock to take care of that, except that the sysfs functions
can sleep.  I'm pretty convinced at this point that the MTU stuff
should go away (handled by userspace if actually needed), which takes
care of that notifier and would allow getting rid of the port_list,
helping the unregister case.

It's possible that we could get into trouble with the vports wanting
to access the DP but they don't care about it except on the packet
processing path (which holds RCU) and in vport_set_mtu().

>> > +static void free_vport_rcu(struct rcu_head *rcu)
>> > +{
>> > + ? ? ? struct vport *vport = container_of(rcu, struct vport, rcu);
>> > +
>> > + ? ? ? if (vport->ops->flags & VPORT_F_GEN_STATS)
>> > + ? ? ? ? ? ? ? free_percpu(vport->percpu_stats);
>> > +
>> > + ? ? ? kobject_put(&vport->kobj);
>> > +}
>> > +
>> > ?/**
>> > ?* ? ? vport_free - uninitialize and free vport
>> > ?*
>> > @@ -212,21 +226,16 @@ struct vport *vport_alloc(int priv_size, const struct vport_ops *ops, const stru
>> > ?*/
>> > ?void vport_free(struct vport *vport)
>> > ?{
>> > - ? ? ? if (vport->ops->flags & VPORT_F_GEN_STATS)
>> > - ? ? ? ? ? ? ? free_percpu(vport->percpu_stats);
>> > -
>> > - ? ? ? kobject_put(&vport->kobj);
>> > + ? ? ? call_rcu(&vport->rcu, free_vport_rcu);
>> > ?}
>>
>> vport_free() is only called from the vport implementation after it has
>> waited for a grace period, so we don't need to wait for another one
>> here.
>
> Are we missing a synchronize_rcu() in internal_dev_destroy()?  It's the
> only ->destroy callback where it isn't obvious to me that vport_free()
> is called only after a grace period.

There's one in unregister_netdevice().  I suppose we should add a comment.

>
> Should we document that the ->destroy function must wait a grace period
> before destroying the vport?

Sure.

> diff --git a/datapath/datapath.h b/datapath/datapath.h
> index 269ac13..e7ce42c 100644
> --- a/datapath/datapath.h
> +++ b/datapath/datapath.h
> @@ -77,8 +77,8 @@ struct dp_stats_percpu {
>  * %ODP_PACKET_CMD_SAMPLE multicast group, e.g. (@sflow_probability/UINT_MAX)
>  * is the probability of sampling a given packet.
>  *
> - * Context: Protected on the read side by rcu_read_lock, on the write side by
> - * genl_mutex.  See the comment on locking at the top of datapath.c.
> + * Context: Different parts are accessed under different contexts.  See the
> + * comment on locking at the top of datapath.c for some information.
>  */

I think if we can't resolve the issues from above about the access
from the notifier code, we really need to document which fields have
different locking.

Also, I noticed that the queue stuff is still there.  We should be
able to get rid of all of it now.

The vport RCU callback is still here but I'm guessing you were just
waiting to hear back about it.




More information about the dev mailing list