[ovs-dev] [PATCH v6 06/12] dpif-netdev: Wait an RCU grace period before freeing ports.

[Daniele Di Proietto]

On 10/04/2016 12:23, "Ben Pfaff" <blp at ovn.org> wrote:

>On Fri, Apr 08, 2016 at 03:12:59AM +0000, Daniele Di Proietto wrote:
>> 
>> 
>> On 01/04/2016 09:52, "Jarno Rajahalme" <jarno at ovn.org> wrote:
>> 
>> >
>> >> On Mar 30, 2016, at 8:08 PM, Daniele Di Proietto
>> >><diproiettod at vmware.com> wrote:
>> >> 
>> >> 
>> >> On 30/03/2016 16:01, "Ben Pfaff" <blp at ovn.org> wrote:
>> >> 
>> >>> (I'm taking a look at this patch specifically because Daniele asked
>>me;
>> >>> I'm not planning to review the whole series.)
>> >>> 
>> >>> On Mon, Mar 28, 2016 at 12:41:40PM -0700, Daniele Di Proietto wrote:
>> >>>> The dpif-netdev datapath keeps ports in a cmap which is written
>>only
>> >>>>by
>> >>>> the main thread (holding port_mutex), but which is read
>>concurrently
>> >>>>by
>> >>>> many threads (most notably the pmd threads).
>> >>>> 
>> >>>> When removing ports from the datapath we should postpone the
>>deletion,
>> >>>> otherwise another thread might access invalid memory while reading
>>the
>> >>>> cmap.
>> >>>> 
>> >>>> This commit splits do_port_del() in do_port_remove() and
>> >>>> do_port_destroy(): the former removes the port from the cmap, while
>> >>>>the
>> >>>> latter reclaims the memory and drops the reference to the
>>underlying
>> >>>> netdev.
>> >>> 
>> >>> s/del_port/port_del/ here:
>> >> 
>> >> Thanks, changed
>> >> 
>> >>> 
>> >>>> dpif_netdev_del_port() now uses ovsrcu_synchronize() before calling
>> >>>> do_port_destroy(), to avoid memory corruption in concurrent
>>readers.
>> >>> 
>> >>> ovsrcu_synchronize() requires that nothing in the thread that calls
>>it
>> >>> is relying on RCU to keep objects around.  That means that no
>>caller of
>> >>> dfpi_port_del()--there are a few of them--can rely on it.  This is
>> >>> usually a risky assumption, especially because this assumption can
>> >>> change later.  Is there reason to believe that it isn't important in
>> >>>all
>> >>> of these cases?
>> >> 
>> >> I agree that's risky, but I think it's the only way to keep the ports
>> >>RCU
>> >> protected, because a port needs to be effectively deleted before
>> >> dpif_netdev_port_del() can return.
>> >> 
>> >
>> >If this is because otherwise a following port_add can fail, as the old
>> >port is still around, maybe we could make the highest possible level of
>> >port_add detect the failure and then rcu_synchronize and try again?
>>Would
>> >that work?
>> >
>> >  Jarno
>> 
>> After some thought I decided to avoid using RCU for ports. I'll send an
>> updated
>> series soon.
>
>Well, that makes the discussion a little easier ;-)  Thanks.
>
>Do you want to me to review anything in the new version?

I was only concerned about the use of RCU. The new version should be
simpler.

Thanks!