On Fri, Oct 11, 2013 at 3:02 PM, Jesse Gross <[email protected]> wrote:
> On Fri, Oct 11, 2013 at 1:03 PM, Alexei Starovoitov <[email protected]> wrote:
>> On Fri, Oct 11, 2013 at 11:11 AM, Jesse Gross <[email protected]> wrote:
>>> On Thu, Oct 10, 2013 at 9:48 PM, Alexei Starovoitov <[email protected]>
>>> wrote:
>>>> On Thu, Oct 10, 2013 at 8:56 PM, Jesse Gross <[email protected]> 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)
>
> Yes, this seems safer. Is the check for NETREG_UNREGISTERING in
> dp_device_event() still needed given that we are checking the event?
at least some check is needed, since NETDEV_UNREGISTER event can
be received again as rebroadcast with reg_state=netreg_unregistered
if wq got delayed.
Probably better to combine checks event == unreg and state == unregistering
under one 'if' to avoid unnecessary workq wakeup.
Or may be better to do it as
if (event == NETDEV_UNREGISTER && dev->priv_flags & IFF_OVS_DATAPATH) {
ovs_netdev_detach_dev();
queue_work();
}
since we're at it... what should be the behavior for namespace moves?
If dev attached to ovs and being moved into a different net namespace, I think
ovs should detach and forget the dev...
Today ovs ignores this notification and we may have ovs-dp in one net
and attached dev
in a different net.
So if you do:
ovs-dpctl add-if test tap1
ip link set tap1 netns 3512
and then try to remove tap1 inside the namespace:
ip tuntap del dev tap1 mode tap
it will just hang:
[ 852.572476] unregister_netdevice: waiting for tap1 to become free.
Usage count = 3
[ 862.578769] unregister_netdevice: waiting for tap1 to become free.
Usage count = 3
>> ovs_netdev_destroy_dev() name instead ovs_netdev_unlink_dev() name?
>
> How about detach_dev?
that's better name indeed. Will respin V2.
Thanks
Alexei
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
