[PATCH 4/6] vlan: Optimize multiple unregistration

[PATCH 4/6] vlan: Optimize multiple unregistration

[Eric Dumazet]

Use unregister_netdevice_many() to speedup master device unregister.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 include/linux/if_vlan.h |    1 
 net/8021q/vlan.c        |   49 +++++++++++++++++++++++++-------------
 net/core/dev.c          |    1 
 3 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 8898cbe..71a4870 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -85,6 +85,7 @@ struct vlan_group {
 					    * the vlan is attached to.
 					    */
 	unsigned int		nr_vlans;
+	int			killall;
 	struct hlist_node	hlist;	/* linked list */
 	struct net_device **vlan_devices_arrays[VLAN_GROUP_ARRAY_SPLIT_PARTS];
 	struct rcu_head		rcu;
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 6b5c9dd..511afe7 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -159,11 +159,12 @@ void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
 	if (real_dev->features & NETIF_F_HW_VLAN_FILTER)
 		ops->ndo_vlan_rx_kill_vid(real_dev, vlan_id);
 
-	vlan_group_set_device(grp, vlan_id, NULL);
 	grp->nr_vlans--;
 
-	synchronize_net();
-
+	if (!grp->killall) {
+		vlan_group_set_device(grp, vlan_id, NULL);
+		synchronize_net();
+	}
 	unregister_netdevice_queue(dev, head);
 
 	/* If the group is now empty, kill off the group. */
@@ -183,6 +184,34 @@ void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
 	dev_put(real_dev);
 }
 
+void unregister_vlan_dev_alls(struct vlan_group *grp)
+{
+	LIST_HEAD(list);
+	int i;
+	struct net_device *vlandev;
+	struct vlan_group save;
+
+	memcpy(&save, grp, sizeof(save));
+	memset(&grp->vlan_devices_arrays, 0, sizeof(grp->vlan_devices_arrays));
+	grp->killall = 1;
+
+	synchronize_net();
+
+	/* Delete all VLANs for this dev. */
+	for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
+		vlandev = vlan_group_get_device(&save, i);
+		if (!vlandev)
+			continue;
+
+		unregister_vlan_dev(vlandev, &list);
+		if (grp->nr_vlans == 0)
+			break;
+	}
+	unregister_netdevice_many(&list);
+	for (i = 0; i < VLAN_GROUP_ARRAY_SPLIT_PARTS; i++)
+		kfree(save.vlan_devices_arrays[i]);
+}
+
 static void vlan_transfer_operstate(const struct net_device *dev,
 				    struct net_device *vlandev)
 {
@@ -524,19 +553,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 		break;
 
 	case NETDEV_UNREGISTER:
-		/* Delete all VLANs for this dev. */
-		for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
-			vlandev = vlan_group_get_device(grp, i);
-			if (!vlandev)
-				continue;
-
-			/* unregistration of last vlan destroys group, abort
-			 * afterwards */
-			if (grp->nr_vlans == 1)
-				i = VLAN_GROUP_ARRAY_LEN;
-
-			unregister_vlan_dev(vlandev, NULL);
-		}
+		unregister_vlan_dev_alls(grp);
 		break;
 	}
 
diff --git a/net/core/dev.c b/net/core/dev.c
index dedacd8..82a3bb9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5303,6 +5303,7 @@ void unregister_netdevice_many(struct list_head *head)
 			net_set_todo(dev);
 	}
 }
+EXPORT_SYMBOL(unregister_netdevice_many);
 
 /**
  *	unregister_netdev - remove device from the kernel

Re: [PATCH 4/6] vlan: Optimize multiple unregistration

[Patrick McHardy]

Eric Dumazet wrote:
> Use unregister_netdevice_many() to speedup master device unregister.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  include/linux/if_vlan.h |    1 
>  net/8021q/vlan.c        |   49 +++++++++++++++++++++++++-------------
>  net/core/dev.c          |    1 
>  3 files changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index 8898cbe..71a4870 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -85,6 +85,7 @@ struct vlan_group {
>  					    * the vlan is attached to.
>  					    */
>  	unsigned int		nr_vlans;
> +	int			killall;
>  	struct hlist_node	hlist;	/* linked list */
>  	struct net_device **vlan_devices_arrays[VLAN_GROUP_ARRAY_SPLIT_PARTS];
>  	struct rcu_head		rcu;
> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
> index 6b5c9dd..511afe7 100644
> --- a/net/8021q/vlan.c
> +++ b/net/8021q/vlan.c
> @@ -159,11 +159,12 @@ void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
>  	if (real_dev->features & NETIF_F_HW_VLAN_FILTER)
>  		ops->ndo_vlan_rx_kill_vid(real_dev, vlan_id);
>  
> -	vlan_group_set_device(grp, vlan_id, NULL);
>  	grp->nr_vlans--;
>  
> -	synchronize_net();
> -
> +	if (!grp->killall) {
> +		vlan_group_set_device(grp, vlan_id, NULL);
> +		synchronize_net();
> +	}
>  	unregister_netdevice_queue(dev, head);
>  
>  	/* If the group is now empty, kill off the group. */
> @@ -183,6 +184,34 @@ void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
>  	dev_put(real_dev);
>  }
>  
> +void unregister_vlan_dev_alls(struct vlan_group *grp)

This could be static.

> +{
> +	LIST_HEAD(list);
> +	int i;
> +	struct net_device *vlandev;
> +	struct vlan_group save;
> +
> +	memcpy(&save, grp, sizeof(save));
> +	memset(&grp->vlan_devices_arrays, 0, sizeof(grp->vlan_devices_arrays));

This shouldn't be necessary since the lower device is already in the
process of being unregistered. If it was necessary, it could cause
crashes since the individual pointers are not set to zero atomically.
Or maybe I'm misunderstanding the purpose entirely :)

> +	grp->killall = 1;
> +
> +	synchronize_net();
> +
> +	/* Delete all VLANs for this dev. */
> +	for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
> +		vlandev = vlan_group_get_device(&save, i);
> +		if (!vlandev)
> +			continue;
> +
> +		unregister_vlan_dev(vlandev, &list);
> +		if (grp->nr_vlans == 0)
> +			break;
> +	}
> +	unregister_netdevice_many(&list);
> +	for (i = 0; i < VLAN_GROUP_ARRAY_SPLIT_PARTS; i++)
> +		kfree(save.vlan_devices_arrays[i]);
> +}
> +
>  static void vlan_transfer_operstate(const struct net_device *dev,
>  				    struct net_device *vlandev)
>  {
> @@ -524,19 +553,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
>  		break;
>  
>  	case NETDEV_UNREGISTER:
> -		/* Delete all VLANs for this dev. */
> -		for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
> -			vlandev = vlan_group_get_device(grp, i);
> -			if (!vlandev)
> -				continue;
> -
> -			/* unregistration of last vlan destroys group, abort
> -			 * afterwards */
> -			if (grp->nr_vlans == 1)
> -				i = VLAN_GROUP_ARRAY_LEN;
> -
> -			unregister_vlan_dev(vlandev, NULL);
> -		}
> +		unregister_vlan_dev_alls(grp);
>  		break;
>  	}
>  

Re: [PATCH 4/6] vlan: Optimize multiple unregistration

[Eric Dumazet]

Patrick McHardy a écrit :
> Eric Dumazet wrote:
>> Use unregister_netdevice_many() to speedup master device unregister.
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> ---
>>  include/linux/if_vlan.h |    1 
>>  net/8021q/vlan.c        |   49 +++++++++++++++++++++++++-------------
>>  net/core/dev.c          |    1 
>>  3 files changed, 35 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>> index 8898cbe..71a4870 100644
>> --- a/include/linux/if_vlan.h
>> +++ b/include/linux/if_vlan.h
>> @@ -85,6 +85,7 @@ struct vlan_group {
>>  					    * the vlan is attached to.
>>  					    */
>>  	unsigned int		nr_vlans;
>> +	int			killall;
>>  	struct hlist_node	hlist;	/* linked list */
>>  	struct net_device **vlan_devices_arrays[VLAN_GROUP_ARRAY_SPLIT_PARTS];
>>  	struct rcu_head		rcu;
>> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
>> index 6b5c9dd..511afe7 100644
>> --- a/net/8021q/vlan.c
>> +++ b/net/8021q/vlan.c
>> @@ -159,11 +159,12 @@ void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
>>  	if (real_dev->features & NETIF_F_HW_VLAN_FILTER)
>>  		ops->ndo_vlan_rx_kill_vid(real_dev, vlan_id);
>>  
>> -	vlan_group_set_device(grp, vlan_id, NULL);
>>  	grp->nr_vlans--;
>>  
>> -	synchronize_net();
>> -
>> +	if (!grp->killall) {
>> +		vlan_group_set_device(grp, vlan_id, NULL);
>> +		synchronize_net();
>> +	}
>>  	unregister_netdevice_queue(dev, head);
>>  
>>  	/* If the group is now empty, kill off the group. */
>> @@ -183,6 +184,34 @@ void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
>>  	dev_put(real_dev);
>>  }
>>  
>> +void unregister_vlan_dev_alls(struct vlan_group *grp)
> 
> This could be static.
> 
>> +{
>> +	LIST_HEAD(list);
>> +	int i;
>> +	struct net_device *vlandev;
>> +	struct vlan_group save;
>> +
>> +	memcpy(&save, grp, sizeof(save));
>> +	memset(&grp->vlan_devices_arrays, 0, sizeof(grp->vlan_devices_arrays));
> 
> This shouldn't be necessary since the lower device is already in the
> process of being unregistered. If it was necessary, it could cause
> crashes since the individual pointers are not set to zero atomically.
> Or maybe I'm misunderstanding the purpose entirely :)

Very good point indeed, even if in practice memset() use long word transferts

I'll make a cleanup patch, or do you want to do it ?

Re: [PATCH 4/6] vlan: Optimize multiple unregistration

[Patrick McHardy]

Eric Dumazet wrote:
> Patrick McHardy a écrit :
>>> +{
>>> +	LIST_HEAD(list);
>>> +	int i;
>>> +	struct net_device *vlandev;
>>> +	struct vlan_group save;
>>> +
>>> +	memcpy(&save, grp, sizeof(save));
>>> +	memset(&grp->vlan_devices_arrays, 0, sizeof(grp->vlan_devices_arrays));
>> This shouldn't be necessary since the lower device is already in the
>> process of being unregistered. If it was necessary, it could cause
>> crashes since the individual pointers are not set to zero atomically.
>> Or maybe I'm misunderstanding the purpose entirely :)
> 
> Very good point indeed, even if in practice memset() use long word transferts
> 
> I'll make a cleanup patch, or do you want to do it ?

I can take care of this, patch will follow shortly.

Re: [PATCH 4/6] vlan: Optimize multiple unregistration

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]

Patrick McHardy wrote:
> Eric Dumazet wrote:
>> Patrick McHardy a écrit :
>>>> +{
>>>> +	LIST_HEAD(list);
>>>> +	int i;
>>>> +	struct net_device *vlandev;
>>>> +	struct vlan_group save;
>>>> +
>>>> +	memcpy(&save, grp, sizeof(save));
>>>> +	memset(&grp->vlan_devices_arrays, 0, sizeof(grp->vlan_devices_arrays));
>>> This shouldn't be necessary since the lower device is already in the
>>> process of being unregistered. If it was necessary, it could cause
>>> crashes since the individual pointers are not set to zero atomically.
>>> Or maybe I'm misunderstanding the purpose entirely :)
>> Very good point indeed, even if in practice memset() use long word transferts
>>
>> I'll make a cleanup patch, or do you want to do it ?
> 
> I can take care of this, patch will follow shortly.

How about this? I moved the code back into vlan_device_event() since
its now only a very minimal change to the original code.

vlan-orig.diff contains the diff between the original code and the
code after this patch for reference.


[-- Attachment #2: vlan.diff --]
[-- Type: text/x-patch, Size: 2891 bytes --]

commit aa8fda37701cb6a452aeb6505c42817ad6e081fc
Author: Patrick McHardy <kaber@trash.net>
Date:   Thu Oct 29 15:13:38 2009 +0100

    vlan: cleanup multiple unregistrations
    
    The temporary copy of the VLAN group is not neccessary since the lower device
    is already in the process of being unregistered, if it was neccessary the
    memset of the global group would introduce a race condition.
    
    With this removed, the changes to the original code are only a few lines, so
    remove the new function and move the code back into vlan_device_event().
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 511afe7..39f8d01 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -161,10 +161,10 @@ void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
 
 	grp->nr_vlans--;
 
-	if (!grp->killall) {
-		vlan_group_set_device(grp, vlan_id, NULL);
+	vlan_group_set_device(grp, vlan_id, NULL);
+	if (!grp->killall)
 		synchronize_net();
-	}
+
 	unregister_netdevice_queue(dev, head);
 
 	/* If the group is now empty, kill off the group. */
@@ -184,34 +184,6 @@ void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
 	dev_put(real_dev);
 }
 
-void unregister_vlan_dev_alls(struct vlan_group *grp)
-{
-	LIST_HEAD(list);
-	int i;
-	struct net_device *vlandev;
-	struct vlan_group save;
-
-	memcpy(&save, grp, sizeof(save));
-	memset(&grp->vlan_devices_arrays, 0, sizeof(grp->vlan_devices_arrays));
-	grp->killall = 1;
-
-	synchronize_net();
-
-	/* Delete all VLANs for this dev. */
-	for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
-		vlandev = vlan_group_get_device(&save, i);
-		if (!vlandev)
-			continue;
-
-		unregister_vlan_dev(vlandev, &list);
-		if (grp->nr_vlans == 0)
-			break;
-	}
-	unregister_netdevice_many(&list);
-	for (i = 0; i < VLAN_GROUP_ARRAY_SPLIT_PARTS; i++)
-		kfree(save.vlan_devices_arrays[i]);
-}
-
 static void vlan_transfer_operstate(const struct net_device *dev,
 				    struct net_device *vlandev)
 {
@@ -456,6 +428,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 	struct vlan_group *grp;
 	int i, flgs;
 	struct net_device *vlandev;
+	LIST_HEAD(list);
 
 	if (is_vlan_dev(dev))
 		__vlan_device_event(dev, event);
@@ -553,7 +526,22 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 		break;
 
 	case NETDEV_UNREGISTER:
-		unregister_vlan_dev_alls(grp);
+		/* Delete all VLANs for this dev. */
+		grp->killall = 1;
+
+		for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
+			vlandev = vlan_group_get_device(grp, i);
+			if (!vlandev)
+				continue;
+
+			/* unregistration of last vlan destroys group, abort
+			 * afterwards */
+			if (grp->nr_vlans == 1)
+				i = VLAN_GROUP_ARRAY_LEN;
+
+			unregister_vlan_dev(vlandev, &list);
+		}
+		unregister_netdevice_many(&list);
 		break;
 	}
 

[-- Attachment #3: vlan-orig.diff --]
[-- Type: text/x-patch, Size: 1399 bytes --]

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 6b5c9dd..39f8d01 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -159,10 +159,11 @@ void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
 	if (real_dev->features & NETIF_F_HW_VLAN_FILTER)
 		ops->ndo_vlan_rx_kill_vid(real_dev, vlan_id);
 
-	vlan_group_set_device(grp, vlan_id, NULL);
 	grp->nr_vlans--;
 
-	synchronize_net();
+	vlan_group_set_device(grp, vlan_id, NULL);
+	if (!grp->killall)
+		synchronize_net();
 
 	unregister_netdevice_queue(dev, head);
 
@@ -427,6 +428,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 	struct vlan_group *grp;
 	int i, flgs;
 	struct net_device *vlandev;
+	LIST_HEAD(list);
 
 	if (is_vlan_dev(dev))
 		__vlan_device_event(dev, event);
@@ -525,6 +527,8 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 
 	case NETDEV_UNREGISTER:
 		/* Delete all VLANs for this dev. */
+		grp->killall = 1;
+
 		for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
 			vlandev = vlan_group_get_device(grp, i);
 			if (!vlandev)
@@ -535,8 +539,9 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 			if (grp->nr_vlans == 1)
 				i = VLAN_GROUP_ARRAY_LEN;
 
-			unregister_vlan_dev(vlandev, NULL);
+			unregister_vlan_dev(vlandev, &list);
 		}
+		unregister_netdevice_many(&list);
 		break;
 	}
 

Re: [PATCH 4/6] vlan: Optimize multiple unregistration

[Eric Dumazet]

Patrick McHardy a écrit :
> Patrick McHardy wrote:
>> Eric Dumazet wrote:
>>> Patrick McHardy a écrit :
>>>>> +{
>>>>> +	LIST_HEAD(list);
>>>>> +	int i;
>>>>> +	struct net_device *vlandev;
>>>>> +	struct vlan_group save;
>>>>> +
>>>>> +	memcpy(&save, grp, sizeof(save));
>>>>> +	memset(&grp->vlan_devices_arrays, 0, sizeof(grp->vlan_devices_arrays));
>>>> This shouldn't be necessary since the lower device is already in the
>>>> process of being unregistered. If it was necessary, it could cause
>>>> crashes since the individual pointers are not set to zero atomically.
>>>> Or maybe I'm misunderstanding the purpose entirely :)
>>> Very good point indeed, even if in practice memset() use long word transferts
>>>
>>> I'll make a cleanup patch, or do you want to do it ?
>> I can take care of this, patch will follow shortly.
> 
> How about this? I moved the code back into vlan_device_event() since
> its now only a very minimal change to the original code.
> 
> vlan-orig.diff contains the diff between the original code and the
> code after this patch for reference.
> 
> 

I have no problem with this solution, but I wonder why you re-added the curious

			/* unregistration of last vlan destroys group, abort
			 * afterwards */
			if (grp->nr_vlans == 1)
				i = VLAN_GROUP_ARRAY_LEN;

			unregister_vlan_dev(vlandev, &list);

while doing

	unregister_vlan_dev(vlandev, &list);
	if (grp->nr_vlans == 0)
		break;

seems more natural :)

Re: [PATCH 4/6] vlan: Optimize multiple unregistration

[Patrick McHardy]

Eric Dumazet wrote:
> Patrick McHardy a écrit :
>> How about this? I moved the code back into vlan_device_event() since
>> its now only a very minimal change to the original code.
>>
>> vlan-orig.diff contains the diff between the original code and the
>> code after this patch for reference.
>>
>>
> 
> I have no problem with this solution, but I wonder why you re-added the curious
> 
> 			/* unregistration of last vlan destroys group, abort
> 			 * afterwards */
> 			if (grp->nr_vlans == 1)
> 				i = VLAN_GROUP_ARRAY_LEN;
> 
> 			unregister_vlan_dev(vlandev, &list);
> 
> while doing
> 
> 	unregister_vlan_dev(vlandev, &list);
> 	if (grp->nr_vlans == 0)
> 		break;
> 
> seems more natural :)

Indeed, but unregister_vlan_dev() destroys the group once the
count has reached zero, so we must not access it after that.

Re: [PATCH 4/6] vlan: Optimize multiple unregistration

[Eric Dumazet]

Patrick McHardy a écrit :

> Indeed, but unregister_vlan_dev() destroys the group once the
> count has reached zero, so we must not access it after that.

Well, I hoped call_rcu() callback doesnt fire and kfree(grp) until we exited
from unregister_vlan_dev_alls(), with RTNL locked...

Re: [PATCH 4/6] vlan: Optimize multiple unregistration

[Patrick McHardy]

Eric Dumazet wrote:
> Patrick McHardy a écrit :
> 
>> Indeed, but unregister_vlan_dev() destroys the group once the
>> count has reached zero, so we must not access it after that.
> 
> Well, I hoped call_rcu() callback doesnt fire and kfree(grp) until we exited
> from unregister_vlan_dev_alls(), with RTNL locked...

The RTNL is a mutex, so it shouldn't prevent call_rcu from firing.

Re: [PATCH 4/6] vlan: Optimize multiple unregistration

[Eric Dumazet]

Patrick McHardy a écrit :
> Eric Dumazet wrote:
>> Patrick McHardy a écrit :
>>
>>> Indeed, but unregister_vlan_dev() destroys the group once the
>>> count has reached zero, so we must not access it after that.
>> Well, I hoped call_rcu() callback doesnt fire and kfree(grp) until we exited
>> from unregister_vlan_dev_alls(), with RTNL locked...
> 
> The RTNL is a mutex, so it shouldn't prevent call_rcu from firing.

Oops this is totally right of course, so your patch is actually a bug fix :)

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Re: [PATCH 4/6] vlan: Optimize multiple unregistration

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 30 Oct 2009 07:30:43 +0100

> Patrick McHardy a écrit :
>> Eric Dumazet wrote:
>>> Patrick McHardy a écrit :
>>>
>>>> Indeed, but unregister_vlan_dev() destroys the group once the
>>>> count has reached zero, so we must not access it after that.
>>> Well, I hoped call_rcu() callback doesnt fire and kfree(grp) until we exited
>>> from unregister_vlan_dev_alls(), with RTNL locked...
>> 
>> The RTNL is a mutex, so it shouldn't prevent call_rcu from firing.
> 
> Oops this is totally right of course, so your patch is actually a bug fix :)
> 
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

I've applied Patrick's patch, thanks everyone.