netdev - Re: [PATCH v2 net-next 2/4] net: dev: add batching to net

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAO7SqHAX7e6sXkcEmd-hAn=Sg4AdNCRS1iFN7_F=aDnidnZOvg@mail.gmail.com>
Date:	Sat, 6 Feb 2016 22:25:20 -0800
From:	Salam Noureddine <noureddine@...sta.com>
To:	Julian Anastasov <ja@....bg>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jiri Pirko <jiri@...lanox.com>,
	Alexei Starovoitov <ast@...mgrid.com>,
	Daniel Borkmann <daniel@...earbox.net>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 net-next 2/4] net: dev: add batching to net_device notifiers

On Sat, Feb 6, 2016 at 10:58 AM, Julian Anastasov <ja@....bg> wrote:

>
>         I now see that we should split the loop
> here, so that NETDEV_DOWN_BATCH is called only once per net:
>
>                 bool down = false;
>
>                 for_each_netdev(net, dev) {
>                         if (dev == last)
>                                 break;
>
>                         if (dev->flags & IFF_UP) {
>                                 call_netdevice_notifier(nb, NETDEV_GOING_DOWN,
>                                                         dev);
>                                 call_netdevice_notifier(nb, NETDEV_DOWN, dev);
>                                 down = true;
>                         }
>                 }
>
>                 rt_cache_flush and arp_ifdown_all will be called
>                 on NETDEV_UNREGISTER_BATCH, so use 'down' flag:
>
>                 if (down)
>                         call_netdevice_notifier(nb, NETDEV_DOWN_BATCH,
>                                                 net->loopback_dev);
>                 for_each_netdev(net, dev) {
>                         if (dev == last)
>                                 goto outroll;
>                         call_netdevice_notifier(nb, NETDEV_UNREGISTER, dev);
>                 }
>                 call_netdevice_notifier(nb, NETDEV_UNREGISTER_BATCH,
>                                         net->loopback_dev);
>
>
>>                       }
>>                       call_netdevice_notifier(nb, NETDEV_UNREGISTER, dev);
>>               }
>> +             call_netdevice_notifier(nb, NETDEV_UNREGISTER_BATCH,
>> +                                     net->loopback_dev);
>>       }
>>
>>  outroll:
>> +     call_netdevice_notifier(nb, NETDEV_UNREGISTER_BATCH, last);
>>       raw_notifier_chain_unregister(&netdev_chain, nb);
>>       goto unlock;
>>  }

I am not sure we need to worry too much about optimizing the failure
code path to justify splitting into two loops.


>>  static void rollback_registered_many(struct list_head *head)
>>  {

>>       list_for_each_entry(dev, head, unreg_list) {
>> -             struct sk_buff *skb = NULL;
>> -
>>               /* Shutdown queueing discipline. */
>>               dev_shutdown(dev);
>>
>> @@ -6475,6 +6497,20 @@ static void rollback_registered_many(struct list_head *head)
>>                  this device. They should clean all the things.
>>               */
>>               call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
>> +     }
>> +
>> +     /* call batch notifiers which act on net namespaces */
>> +     list_for_each_entry(dev, head, unreg_list) {
>> +             net_add_event_list(&net_head, dev_net(dev));
>
>         Looks like we can move the above net_add_event_list with
> the comment into the previous loop after NETDEV_UNREGISTER,
> we will save some cycles.
>

I didn't move it into the previous loop because the NETDEV_UNREGISTER
notifier can
end up calling rollback_registered_many (for example the vlan driver
unregistering
all vlans on top of a device) in which case we would be using the
event_list in the net
namespace.

> Julian Anastasov <ja@....bg>

Thanks,

Salam