[ovs-dev] [PATCH net-next] openvswitch: fix vport-netdev unregister

[Alexei Starovoitov]

On Fri, Oct 11, 2013 at 11:11 AM, Jesse Gross <jesse at nicira.com> wrote:
> On Thu, Oct 10, 2013 at 9:48 PM, Alexei Starovoitov <ast at plumgrid.com> wrote:
>> On Thu, Oct 10, 2013 at 8:56 PM, Jesse Gross <jesse at nicira.com> wrote:
>>> However, the check dev->reg_state in netdev_destroy() looks racy to
>>> me, as it could already be in NETREG_UNREGISTERED even if we already
>>> processed this device.
>>
>> you mean that netdev_destroy() will see reg_state == netreg_unregistered,
>> while dp_device_event() didn't see reg_state == netreg_unregistering yet?
>> or dp_device_event() saw it, proceeded to do unlink and
>> netdev_destroy() ran in parallel?
>> well, that's why reg_state == netreg_unregistering check in netdev_destroy()
>> is done with rtnl_lock() held.
>> reg_state cannot go into netreg_unregistered state skipping
>> netreg_unregistering and notifier.
>> therefore I don't think it's racy.
>>
>> In ovs_dp_notify_wq() you're checking for both unregistering and
>> unregistered and that makes
>> sense, since workq can run after unregistering notifier called and
>> netdev_run_todo()
>> already changed the state to unregistered.
>> But here it's not the case.
>
> ovs_dp_notify_wq() calls ovs_dp_detach_port(), which indirectly calls
> netdev_destroy() so it seems like it actually is the same case to me.

yes. makes sense.
how about:
-       if (netdev_vport->dev->reg_state != NETREG_UNREGISTERING)
+       if (netdev_vport->dev->priv_flags & IFF_OVS_DATAPATH)

or you would prefer (state != unregistering && state != unregistered)
check instead?

ovs_netdev_destroy_dev() name instead ovs_netdev_unlink_dev() name?

Thanks
Alexei