Re: [PATCH v3 net-next] net: vxlan: when lower dev unregisters remove vxlan dev as well

[Daniel Borkmann <dborkman@redhat.com>]

On 01/11/2014 08:03 PM, Cong Wang wrote:
> On Sat, Jan 11, 2014 at 12:46 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
>> On 01/11/2014 01:49 AM, Cong Wang wrote:
>>>
>>> On Fri, Jan 10, 2014 at 2:35 PM, Daniel Borkmann <dborkman@redhat.com>
>>> wrote:
>>>>
>>>> @@ -2673,13 +2712,14 @@ static __net_init int vxlan_init_net(struct net
>>>> *net)
>>>>    static __net_exit void vxlan_exit_net(struct net *net)
>>>>    {
>>>>           struct vxlan_net *vn = net_generic(net, vxlan_net_id);
>>>> -       struct vxlan_dev *vxlan;
>>>> -       LIST_HEAD(list);
>>>> +       struct vxlan_dev *vxlan, *next;
>>>> +       LIST_HEAD(list_kill);
>>>>
>>>>           rtnl_lock();
>>>> -       list_for_each_entry(vxlan, &vn->vxlan_list, next)
>>>> -               unregister_netdevice_queue(vxlan->dev, &list);
>>>> -       unregister_netdevice_many(&list);
>>>> +       list_for_each_entry_safe(vxlan, next, &vn->vxlan_list, next)
>>>> +               vxlan_dellink(vxlan->dev, &list_kill);
>>>> +       unregister_netdevice_many(&list_kill);
>>>> +       list_del(&list_kill);
>>>
>>>
>>> The last list_del() looks suspicous... Since list_kill is a local list
>>> head,
>>> why do we need to delete the head at the end??
>>
>>
>> Cong, maybe I'm missing something, but we're doing this rtnl_dellink()
>> and elsewehere, e.g. commit 226bd341147 ("net: use batched device unregister
>> in veth and macvlan").
>
> list_kill is a list *head* (not a node) on stack. unregister_netdevice_many()
> should remove all the nodes in this list after it finishes. So, list_kill is
> supposed to be empty after that.
>
> Either unregister_netdevice_many() is mis-designed, i.e. it is not like what I
> said above, or list_del() is completely unnecessary.
>
> BTW, there are many places calling unregister_netdevice_many() without
> a following list_del(). For example, bond_net_exit().

Basically I agree with you; I'll wait if there are more comments coming up
that address this question, if not I'll respin, remove that and split the patch
into two parts by the beginning of next week.

Cheers,

Daniel