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

Jesse Gross jesse at nicira.com
Fri Jan 21 01:10:23 UTC 2011


On Thu, Jan 20, 2011 at 3:12 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Wed, Jan 19, 2011 at 07:00:01PM -0800, Jesse Gross wrote:
>> On Tue, Jan 11, 2011 at 9:49 PM, Ben Pfaff <blp at nicira.com> wrote:
>> Some other notes on locking issues that I ran across while reviewing this:
>> * dp->port_list uses the *_rcu variants of the list functions but it
>> is not actually protected by RCU.  It is actually protected by
>> dp_mutex.
>
> OK, I changed the two instances of list_for_each_entry_rcu() in
> datapath.c that were iterating through port_list to use
> list_for_each_entry() instead.

There's also one instance of list_add_rcu().

>> > ?* ? ? vport_locate - find a port that has already been created
>> > ?*
>> > ?* @name: name of port to find
>> > - *
>> > - * Either RTNL or vport lock must be acquired before calling this function
>> > - * and held while using the found port. ?See the locking comments at the
>> > - * top of the file.
>> > ?*/
>> > ?struct vport *vport_locate(const char *name)
>> > ?{
>> > @@ -199,11 +148,6 @@ struct vport *vport_locate(const char *name)
>> > ? ? ? ?struct vport *vport;
>> > ? ? ? ?struct hlist_node *node;
>> >
>> > - ? ? ? if (unlikely(!mutex_is_locked(&vport_mutex) && !rtnl_is_locked())) {
>> > - ? ? ? ? ? ? ? pr_err("neither RTNL nor vport lock held in vport_locate\n");
>> > - ? ? ? ? ? ? ? dump_stack();
>> > - ? ? ? }
>> > -
>> > ? ? ? ?rcu_read_lock();
>>
>> It's not clear to me what is handling the locking for vports.  It
>> appears to be dp_mutex but none of the vport functions are documented
>> as requiring it.  The add/del functions are protected by RTNL but
>> locate can't be because it is called by get_datapath() which does not
>> (and should not) hold it.
>>
>> At the very least the vport functions should be documented as to the
>> locking that they expect.  Perhaps a better way would be to convert
>> the vport hash table to use RCU and then there would be one less lock
>> to worry about in the vport layer.
>
> All of them are protected by dp_mutex, including get_datapath().  Shall
> I add comments to that effect?

Yes, please comment it.

>
> Here's the incremental.  It does include the vport_del_all() deletion
> but I'll break that out separately.

Can you please also comment the locking expectations for the functions
in datapath.c?  When I reviewed it I had to look at all of the callers
for all of the functions to know what was expected.

Otherwise the diff looks good.

> I also added use of rcu_dereference_{check,protected}() in table.c.  Now
> a basic run with a simple GRE tunnel setup is lockdep-clean for me.

Great.  I don't think it is entirely correct to check dp_mutex for the
tunnel table since it is really protected by RTNL but it's good enough
and useful for it to not be spewing lockdep errors.




More information about the dev mailing list