[ovs-dev] [PATCH 18/19] datapath: Hold mutex for DP while userspace is blocking on read.

[Jesse Gross]

On Thu, Dec 9, 2010 at 10:22 AM, Ben Pfaff <blp at nicira.com> wrote:
> On Wed, Dec 08, 2010 at 10:14:16PM -0800, Jesse Gross wrote:
>> Currently we get a pointer to the DP in openvswitch_read() and
>> openvswitch_poll() and use it without any synchronization.  This means
>> that the DP could disappear from underneath us while we are using it.
>> Currently, this isn't a problem because userspace is single threaded but
>> it's better for the locking to be correct.
>>
>> With this change we hold the mutex while doing a blocking wait, which
>> means that no changes can be made, including adding/removing flows.  It's
>> possible to make this finer grained but for the time being that isn't done,
>> since current userspace doesn't care.
>>
>> Found with lockdep.
>>
>> Signed-off-by: Jesse Gross <jesse at nicira.com>
>
> I'm only OK with this because:
>
>        1. We're going to get rid of this function and thus this locking
>           strategy for it anyway with the move to Netlink.
>
>        2. Current userspace always opens these devices in nonblocking
>           mode anyway.

I agree, it's not great but I wasn't that interested in optimizing
something that's going away.  I did want to at least make it correct
though.

>
> It's really weird that I haven't seen this lockdep warning.  I always
> run with lockdep enabled.  I wonder if there's something wonky with it,
> it puzzled me why didn't report something else a few days ago too.

The version in master won't flag this, you need the extra checks
introduced in 2.6.37 that I've started using locally.  In this case,
get_dp() is documented as requiring either rcu_read_lock() or
dp_mutex, so I changed the dereference to:

rcu_dereference_check(dps[dp_idx], rcu_read_lock_held() ||
					 lockdep_is_held(&dp_mutex))

Also, by default lockdep only flags the first RCU warning and we have
a lot of false positives currently (like the one you mentioned the
other day), so it reported that and stopped.