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]

Message-ID: <alpine.LFD.2.11.1602030950280.1736@ja.home.ssi.bg>
Date:	Wed, 3 Feb 2016 10:08:26 +0200 (EET)
From:	Julian Anastasov <ja@....bg>
To:	Salam Noureddine <noureddine@...sta.com>
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


	Hello,

On Tue, 2 Feb 2016, Salam Noureddine wrote:

> On Tue, Feb 2, 2016 at 12:55 PM, Julian Anastasov <ja@....bg> wrote:
> >
> >                         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.

	Yes, the idea is to avoid NETDEV_UNREGISTER_BATCH for
every dev. And not to forget to call it for every net.
In this case it is a cleanup path after failure.

> >>                       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.

	Agreed

> >> +     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();
> 
> 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.

	Aha, I see, it is after NETDEV_UNREGISTER but may be
the above loop should be changed to two loops so that
NETDEV_UNREGISTER_BATCH is called exactly after all
NETDEV_UNREGISTER and before all dev_*_flush and
ndo_uninit calls to avoid any risks. I mean:

        synchronize_net();

	First part of loop:

        list_for_each_entry(dev, head, unreg_list) {
                /* Shutdown queueing discipline. */
                dev_shutdown(dev);

                /* Notify protocols, that we are about to destroy
                   this device. They should clean all the things.
                */
                call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
	}

	This is the same NETDEV_UNREGISTER_BATCH logic:

+       list_for_each_entry(dev, head, unreg_list) {
+               net_add_event_list(&net_head, dev_net(dev));
+       }
+       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);
+       }

	Second part of the loop:

        list_for_each_entry(dev, head, unreg_list) {
                struct sk_buff *skb = NULL;

		if (!dev->rtnl_link_ops ||
		...

Regards

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