Skip to contentSkip to conversation

vlan: Optimize multiple unregistration

PATCH Applied Eric Dumazet 3 participants 11 messages Oct 27, 2009 – Oct 30, 2009

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Eric Dumazet

author
#0 lore raw

PATCH 4/6 vlan: Optimize multiple unregistration

Use unregister_netdevice_many() to speedup master device unregister.

Signed-off-by Eric Dumazet <[email protected]>
---
 include/linux/if_vlan.h |    1 
 net/8021q/vlan.c        |   49 +++++++++++++++++++++++++-------------
 net/core/dev.c          |    1 
 3 files changed, 35 insertions(+), 16 deletions(-)
include/linux/if_vlan.h +1 -0
index 8898cbe..71a4870 100644
@@ -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;
net/8021q/vlan.c +33 -16

69 lines not on this page · raw message

net/core/dev.c +1 -0
index dedacd8..82a3bb9 100644
@@ -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
#1 lore raw
Quoted 42 lines · Eric Dumazet

Use unregister_netdevice_many() to speedup master device unregister.

Signed-off-by Eric Dumazet <[email protected]>
---
 include/linux/if_vlan.h |    1 
 net/8021q/vlan.c        |   49 +++++++++++++++++++++++++-------------
 net/core/dev.c          |    1 
 3 files changed, 35 insertions(+), 16 deletions(-)
include/linux/if_vlan.h +1 -0
index 8898cbe..71a4870 100644
@@ -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;
net/8021q/vlan.c +4 -3
index 6b5c9dd..511afe7 100644
@@ -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. */

net/8021q/vlan.c void unregister_vlan_dev(struct net_device *dev, struct list_head *head) · line 187

Quoted 4 lines
net/8021q/vlan.c +1 -0
@@ -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.

net/8021q/vlan.c void unregister_vlan_dev(struct net_device *dev, struct list_head *head) · line 195

Quoted 8 lines
net/8021q/vlan.c +8 -0
+{
+	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 :)

Quoted 43 lines
net/8021q/vlan.c +20 -13
+	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;
 	}
author
#2 lore raw
Quoted 50 lines · Patrick McHardy
Quoted 42 lines · Eric Dumazet

Use unregister_netdevice_many() to speedup master device unregister.

Signed-off-by Eric Dumazet <[email protected]>
---
 include/linux/if_vlan.h |    1 
 net/8021q/vlan.c        |   49 +++++++++++++++++++++++++-------------
 net/core/dev.c          |    1 
 3 files changed, 35 insertions(+), 16 deletions(-)
include/linux/if_vlan.h +1 -0
index 8898cbe..71a4870 100644
@@ -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;
net/8021q/vlan.c +4 -3
index 6b5c9dd..511afe7 100644
@@ -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. */

net/8021q/vlan.c void unregister_vlan_dev(struct net_device *dev, struct list_head *head) · line 187

Quoted 4 lines
net/8021q/vlan.c +1 -0
@@ -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.

Quoted 13 lines

net/8021q/vlan.c void unregister_vlan_dev(struct net_device *dev, struct list_head *head) · line 195

Quoted 8 lines
net/8021q/vlan.c +8 -0
+{
+	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 ?

#3 lore raw
Quoted 14 lines · Eric Dumazet
Quoted 4 lines · Patrick McHardy
Quoted 4 lines
net/8021q/vlan.c +4 -0
+{
+	LIST_HEAD(list);
+	int i;
+	struct net_device *vlandev;
Quoted 8 lines

net/8021q/vlan.c void unregister_vlan_dev(struct net_device *dev, struct list_head *head) · line 195

Quoted 4 lines
net/8021q/vlan.c +4 -0
+	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 :)

Quoted 3 lines

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.

#4 lore raw
Quoted 14 lines · Patrick McHardy
Quoted 13 lines · Eric Dumazet
Quoted 12 lines · Patrick McHardy

Review

Quoted 8 lines
(quoted diff) +8 -0
+{
+	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 :)

Quoted 5 lines
Quoted 3 lines

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.

commit aa8fda37701cb6a452aeb6505c42817ad6e081fc

Author: Patrick McHardy <[email protected]>
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 <[email protected]>
net/8021q/vlan.c +20 -32
index 511afe7..39f8d01 100644
@@ -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;
 	}
net/8021q/vlan.c +8 -3
index 6b5c9dd..39f8d01 100644
@@ -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;
 	}
author
#5 lore raw
Quoted 17 lines · Patrick McHardy
Quoted 16 lines · Patrick McHardy
Quoted 15 lines · Eric Dumazet
Quoted 4 lines · Patrick McHardy
Quoted 4 lines
(quoted diff) +4 -0
+{
+	LIST_HEAD(list);
+	int i;
+	struct net_device *vlandev;
Quoted 8 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+	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

Quoted 10 lines
Quoted 2 lines
Quoted 1 line

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

#6 lore raw
Quoted 18 lines · Eric Dumazet
Quoted 7 lines · Patrick McHardy

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

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.

author
#7 lore raw
Quoted 2 lines · Patrick McHardy

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

#8 lore raw
Quoted 7 lines · Eric Dumazet
Quoted 2 lines · Patrick McHardy

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.

author Acked-by
#9 lore raw
Quoted 9 lines · Patrick McHardy
Quoted 6 lines · Eric Dumazet
Quoted 2 lines · Patrick McHardy

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 <[email protected]>
maintainer Applied
#10 lore raw

From: Eric Dumazet <[email protected]>
Date: Fri, 30 Oct 2009 07:30:43 +0100

Quoted 6 lines
Quoted 5 lines · Patrick McHardy
Quoted 4 lines · Eric Dumazet
Quoted 2 lines · Patrick McHardy

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

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

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 <[email protected]>

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