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 [off-list ref] wrote:

        Hello,

On Mon, 1 Feb 2016, Salam Noureddine wrote:

quoted

@@ -1572,8 +1582,12 @@ rollback:

quoted

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

quoted

              }

                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);

quoted

      }

@@ -1614,8 +1628,12 @@ int unregister_netdevice_notifier(struct notifier_block *nb)

quoted

                      call_netdevice_notifier(nb, NETDEV_UNREGISTER, dev);
+                     call_netdevice_notifier(nb, NETDEV_UNREGISTER_BATCH,
+                                             dev);

                        Above call...

quoted

              }

                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.

quoted

+     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

quoted

      synchronize_net();

      list_for_each_entry(dev, head, unreg_list)

Regards

--
Julian Anastasov [off-list ref]

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