netdev - Re: [PATCH 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]

Date:	Tue, 2 Feb 2016 22:31:06 -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 net-next 2/4] net: dev: add batching to net_device notifiers

On Tue, Feb 2, 2016 at 12:55 PM, Julian Anastasov <ja@....bg> wrote:
>
>         Hello,
>
> On Mon, 1 Feb 2016, Salam Noureddine wrote:
>
>> @@ -1572,8 +1582,12 @@ rollback:

>>                       call_netdevice_notifier(nb, NETDEV_UNREGISTER, dev);
>> +                     call_netdevice_notifier(nb, NETDEV_UNREGISTER_BATCH,
>> +                                             dev);
>
>                         If the rule is once per net, the above call...
>
>>               }
>
>                 should be here:
>
>                 call_netdevice_notifier(nb, NETDEV_UNREGISTER_BATCH,
>                                         net->loopback_dev);
>
>         and also once after outroll label?:

That's a good optimization to add. I was mostly focusing on the device
unregister path.

>
>         call_netdevice_notifier(nb, NETDEV_UNREGISTER_BATCH, last);
>
>>       }
>>
>> @@ -1614,8 +1628,12 @@ int unregister_netdevice_notifier(struct notifier_block *nb)

>>                       call_netdevice_notifier(nb, NETDEV_UNREGISTER, dev);
>> +                     call_netdevice_notifier(nb, NETDEV_UNREGISTER_BATCH,
>> +                                             dev);
>
>                         Above call...
>
>>               }
>
>                 should be here, for net->loopback_dev?
> Also, is it ok to call NETDEV_DOWN_BATCH many times, as result,
> sometimes after NETDEV_UNREGISTER?

Same here, I can add this optimization. I think it is fine to call the
BATCH notifiers
for every interface. It is just better to do it for many interfaces at
the same time.


>> +     list_for_each_entry_safe(net, net_tmp, &net_head, event_list) {
>> +             call_netdevice_notifiers(NETDEV_UNREGISTER_BATCH,
>> +                                      net->loopback_dev);
>> +             net_del_event_list(net);
>> +     }
>> +
>
>         NETDEV_UNREGISTER* should not be called before
> following synchronize_net and NETDEV_UNREGISTER. May be
> we should split the loop: loop (dev_shutdown+NETDEV_UNREGISTER)
> followed by above NETDEV_UNREGISTER_BATCH then again the
> loop for all remaining calls
>
>>       synchronize_net();
>>
>>       list_for_each_entry(dev, head, unreg_list)
>
> Regards
>
> --
> Julian Anastasov <ja@....bg>

The call to NETDEV_UNREGISTER_BATCH is actually after NETDEV_UNREGISTER,
it seems the other way around in the patch because it is showing part
of netdev_wait_allrefs
and not rollback_registered_may.

Thanks,

Salam