Re: [PATCH net-next 2/4] net: dev: add batching to net_device notifiers

[Julian Anastasov]

Hello,

On Tue, 2 Feb 2016, Salam Noureddine wrote:

> On Tue, Feb 2, 2016 at 12:55 PM, Julian Anastasov  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, _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(_head, dev_net(dev));
+   }
+   list_for_each_entry_safe(net, net_tmp, _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 

Re: [PATCH net-next 2/4] net: dev: add batching to net_device notifiers

[Salam Noureddine]

On Wed, Feb 3, 2016 at 12:08 AM, Julian Anastasov  wrote:


> 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(_head, dev_net(dev));
> +   }
> +   list_for_each_entry_safe(net, net_tmp, _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 

You're right. It is probably safer to organize the code the way you said.
Will change that, Thanks!

Salam

Re: [PATCH net-next 2/4] net: dev: add batching to net_device notifiers

[Julian Anastasov]

Hello,

On Mon, 1 Feb 2016, Salam Noureddine wrote:

> @@ -1572,8 +1582,12 @@ rollback:
>   call_netdevice_notifier(nb, NETDEV_GOING_DOWN,
>   dev);
>   call_netdevice_notifier(nb, NETDEV_DOWN, dev);
> + call_netdevice_notifier(nb, NETDEV_DOWN_BATCH,
> + dev);
>   }
>   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?:

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_GOING_DOWN,
>   dev);
>   call_netdevice_notifier(nb, NETDEV_DOWN, dev);
> + call_netdevice_notifier(nb, NETDEV_DOWN_BATCH,
> + dev);
>   }
>   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?

>   }
>  unlock:

> @@ -6427,7 +6447,9 @@ static void net_set_todo(struct net_device *dev)
>  static void rollback_registered_many(struct list_head *head)
>  {
>   struct net_device *dev, *tmp;
> + struct net *net, *net_tmp;
>   LIST_HEAD(close_head);
> + LIST_HEAD(net_head);
>  
>   BUG_ON(dev_boot_phase);
>   ASSERT_RTNL();
> @@ -6504,6 +6526,15 @@ static void rollback_registered_many(struct list_head 
> *head)
>  #endif
>   }
>  
> + list_for_each_entry(dev, head, unreg_list) {
> + net_add_event_list(_head, dev_net(dev));
> + }
> + list_for_each_entry_safe(net, net_tmp, _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 

Re: [PATCH net-next 2/4] net: dev: add batching to net_device notifiers

[Salam Noureddine]

On Tue, Feb 2, 2016 at 12:55 PM, Julian Anastasov  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, _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 

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