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

Jarno Rajahalme jarno at ovn.org
Fri Apr 1 16:52:39 UTC 2016


> 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




More information about the dev mailing list