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