[patch 7/11] net: Use bigrefs for net_device.refcount

The net_device has a refcnt used to keep track of it's uses.
This is used at the time of unregistering the network device
(module unloading ..) (see netdev_wait_allrefs) .
For loopback_dev , this refcnt increment/decrement  is causing
unnecessary traffic on the interlink for NUMA system
affecting it's performance.  This patch improves tbench numbers by 6% on a
8way x86 Xeon (x445).

This patch is dependent on the bigref patch 

Signed-off-by : Niraj Kumar [off-list ref]
Signed-off-by : Shai Fultheim [off-list ref]
Signed-off-by : Ravikiran Thirumalai [off-list ref]

Index: alloc_percpu-2.6.13/drivers/net/loopback.c
===================================================================

--- alloc_percpu-2.6.13.orig/drivers/net/loopback.c	2005-08-28 16:41:01.000000000 -0700
+++ alloc_percpu-2.6.13/drivers/net/loopback.c	2005-09-12 12:04:25.000000000 -0700

@@ -226,6 +226,12 @@
 		loopback_dev.priv = stats;
 		loopback_dev.get_stats = &get_stats;
 	}
+
+	/* 
+	 * This is the only struct net_device not allocated by alloc_netdev
+	 * So explicitly init the bigref hanging off loopback_dev
+	 */
+	bigref_init(&loopback_dev.netdev_refcnt);
 	
 	return register_netdev(&loopback_dev);
 };

Index: alloc_percpu-2.6.13/include/linux/netdevice.h
===================================================================

--- alloc_percpu-2.6.13.orig/include/linux/netdevice.h	2005-08-28 16:41:01.000000000 -0700
+++ alloc_percpu-2.6.13/include/linux/netdevice.h	2005-09-12 11:54:21.000000000 -0700

@@ -37,6 +37,7 @@
 #include <linux/config.h>
 #include <linux/device.h>
 #include <linux/percpu.h>
+#include <linux/bigref.h>
 
 struct divert_blk;
 struct vlan_group;

@@ -377,7 +378,7 @@
 	/* device queue lock */
 	spinlock_t		queue_lock;
 	/* Number of references to this device */
-	atomic_t		refcnt;
+	struct bigref	        netdev_refcnt;	
 	/* delayed register/unregister */
 	struct list_head	todo_list;
 	/* device name hash chain */

@@ -677,11 +678,11 @@
 
 static inline void dev_put(struct net_device *dev)
 {
-	atomic_dec(&dev->refcnt);
+	bigref_put(&dev->netdev_refcnt, NULL);
 }
 
-#define __dev_put(dev) atomic_dec(&(dev)->refcnt)
-#define dev_hold(dev) atomic_inc(&(dev)->refcnt)
+#define __dev_put(dev) bigref_put(&(dev)->netdev_refcnt, NULL);
+#define dev_hold(dev) bigref_get(&(dev)->netdev_refcnt);
 
 /* Carrier loss detection, dial on demand. The functions netif_carrier_on
  * and _off may be called from IRQ context, but it is caller

Index: alloc_percpu-2.6.13/net/core/dev.c
===================================================================

--- alloc_percpu-2.6.13.orig/net/core/dev.c	2005-08-28 16:41:01.000000000 -0700
+++ alloc_percpu-2.6.13/net/core/dev.c	2005-09-12 11:54:21.000000000 -0700

@@ -2658,6 +2658,7 @@
 		goto out;
 
 	dev->iflink = -1;
+	bigref_set(&dev->netdev_refcnt, 0);
 
 	/* Init, if this function is available */
 	if (dev->init) {

@@ -2808,7 +2809,7 @@
 	unsigned long rebroadcast_time, warning_time;
 
 	rebroadcast_time = warning_time = jiffies;
-	while (atomic_read(&dev->refcnt) != 0) {
+	while ( bigref_val(&dev->netdev_refcnt) != 0) {
 		if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
 			rtnl_shlock();
 

@@ -2838,7 +2839,7 @@
 			printk(KERN_EMERG "unregister_netdevice: "
 			       "waiting for %s to become free. Usage "
 			       "count = %d\n",
-			       dev->name, atomic_read(&dev->refcnt));
+			       dev->name, bigref_val(&dev->netdev_refcnt));
 			warning_time = jiffies;
 		}
 	}

@@ -2909,7 +2910,7 @@
 			netdev_wait_allrefs(dev);
 
 			/* paranoia */
-			BUG_ON(atomic_read(&dev->refcnt));
+			BUG_ON(bigref_val(&dev->netdev_refcnt));
 			BUG_TRAP(!dev->ip_ptr);
 			BUG_TRAP(!dev->ip6_ptr);
 			BUG_TRAP(!dev->dn_ptr);

@@ -2969,6 +2970,7 @@
 
 	setup(dev);
 	strcpy(dev->name, name);
+	bigref_init(&dev->netdev_refcnt);
 	return dev;
 }
 EXPORT_SYMBOL(alloc_netdev);

@@ -2986,6 +2988,7 @@
 #ifdef CONFIG_SYSFS
 	/*  Compatiablity with error handling in drivers */
 	if (dev->reg_state == NETREG_UNINITIALIZED) {
+		bigref_destroy(&dev->netdev_refcnt);
 		kfree((char *)dev - dev->padded);
 		return;
 	}

@@ -2996,6 +2999,7 @@
 	/* will free via class release */
 	class_device_put(&dev->class_dev);
 #else
+	bigref_destroy(&dev->netdev_refcnt);
 	kfree((char *)dev - dev->padded);
 #endif
 }

@@ -3210,7 +3214,7 @@
 		set_bit(__LINK_STATE_START, &queue->backlog_dev.state);
 		queue->backlog_dev.weight = weight_p;
 		queue->backlog_dev.poll = process_backlog;
-		atomic_set(&queue->backlog_dev.refcnt, 1);
+		bigref_init(&queue->backlog_dev.netdev_refcnt);
 	}
 
 	dev_boot_phase = 0;

Index: alloc_percpu-2.6.13/net/core/net-sysfs.c
===================================================================

--- alloc_percpu-2.6.13.orig/net/core/net-sysfs.c	2005-08-28 16:41:01.000000000 -0700
+++ alloc_percpu-2.6.13/net/core/net-sysfs.c	2005-09-12 11:54:21.000000000 -0700

@@ -16,6 +16,7 @@
 #include <net/sock.h>
 #include <linux/rtnetlink.h>
 #include <linux/wireless.h>
+#include <linux/bigref.h>
 
 #define to_class_dev(obj) container_of(obj,struct class_device,kobj)
 #define to_net_dev(class) container_of(class, struct net_device, class_dev)

@@ -400,6 +401,8 @@
 		= container_of(cd, struct net_device, class_dev);
 
 	BUG_ON(dev->reg_state != NETREG_RELEASED);
+	
+	bigref_destroy(&dev->netdev_refcnt);
 
 	kfree((char *)dev - dev->padded);
 }

On Tue, 13 Sep 2005 09:10:12 -0700
Ravikiran G Thirumalai [off-list ref] wrote:

The net_device has a refcnt used to keep track of it's uses.
This is used at the time of unregistering the network device
(module unloading ..) (see netdev_wait_allrefs) .
For loopback_dev , this refcnt increment/decrement  is causing
unnecessary traffic on the interlink for NUMA system
affecting it's performance.  This patch improves tbench numbers by 6% on a
8way x86 Xeon (x445).

Since when is bringing a network device up/down performance critical?

Stephen Hemminger wrote:

On Tue, 13 Sep 2005 09:10:12 -0700
Ravikiran G Thirumalai [off-list ref] wrote:

quoted

The net_device has a refcnt used to keep track of it's uses.
This is used at the time of unregistering the network device
(module unloading ..) (see netdev_wait_allrefs) .
For loopback_dev , this refcnt increment/decrement  is causing
unnecessary traffic on the interlink for NUMA system
affecting it's performance.  This patch improves tbench numbers by 6% on a
8way x86 Xeon (x445).

Since when is bringing a network device up/down performance critical?

We grab and drop a reference for each poll of a device, roughly.

See dev_hold in _netif_rx_schedule(struct net_device *dev)
in include/netdevice.h, for instance.

Thanks,
Ben

-- 
Ben Greear [off-list ref]
Candela Technologies Inc  http://www.candelatech.com

On Tue, 13 Sep 2005 09:35:14 -0700
Ben Greear [off-list ref] wrote:

Stephen Hemminger wrote:

quoted

On Tue, 13 Sep 2005 09:10:12 -0700
Ravikiran G Thirumalai [off-list ref] wrote:

quoted

The net_device has a refcnt used to keep track of it's uses.
This is used at the time of unregistering the network device
(module unloading ..) (see netdev_wait_allrefs) .
For loopback_dev , this refcnt increment/decrement  is causing
unnecessary traffic on the interlink for NUMA system
affecting it's performance.  This patch improves tbench numbers by 6% on a
8way x86 Xeon (x445).

Since when is bringing a network device up/down performance critical?

We grab and drop a reference for each poll of a device, roughly.

See dev_hold in _netif_rx_schedule(struct net_device *dev)
in include/netdevice.h, for instance.

Yeah, that would be an issue, especially since the rest of that
path is nicely per-cpu

Ravikiran G Thirumalai a écrit :

The net_device has a refcnt used to keep track of it's uses.
This is used at the time of unregistering the network device
(module unloading ..) (see netdev_wait_allrefs) .
For loopback_dev , this refcnt increment/decrement  is causing
unnecessary traffic on the interlink for NUMA system
affecting it's performance.  This patch improves tbench numbers by 6% on a
8way x86 Xeon (x445).

  ===================================================================

Hum...

Did you tried to place refcnt/netdev_refcnt in a separate cache line than 
queue_lock ? I got good results too...

 >  	/* device queue lock */
 >  	spinlock_t		queue_lock;
 >  	/* Number of references to this device */
 > -	atomic_t		refcnt;
 > +	struct bigref	        netdev_refcnt ____cacheline_aligned_in_smp ;	
 >  	/* delayed register/unregister */
 >  	struct list_head	todo_list;
 >  	/* device name hash chain */

Every time a cpu take the queue_lock spinlock, it exclusively gets one cache 
line. If another cpu try to access netdev_refcnt, it has to grab this cache 
line (even if properely per_cpu designed, there is still one shared field). In 
fact the whole struct net_device should be re-ordered for SMP/NUMA performance.

Eric

On Tue, Sep 13, 2005 at 08:27:52PM +0200, Eric Dumazet wrote:

Ravikiran G Thirumalai a écrit :

Hum...

Did you tried to place refcnt/netdev_refcnt in a separate cache line than 
queue_lock ? I got good results too...

quoted

 	/* device queue lock */
 	spinlock_t		queue_lock;
 	/* Number of references to this device */
-	atomic_t		refcnt;
+	struct bigref	        netdev_refcnt ____cacheline_aligned_in_smp ; 
 	/* delayed register/unregister */
 	struct list_head	todo_list;
 	/* device name hash chain */

Every time a cpu take the queue_lock spinlock, it exclusively gets one 
cache line. If another cpu try to access netdev_refcnt, it has to grab this 
cache line (even if properely per_cpu designed, there is still one shared 
field). In fact the whole struct net_device should be re-ordered for 
SMP/NUMA performance.

I agree. Maybe placing the queue_lock in a different cacheline is the 
right approach?

Thanks,
Kiran

From: Stephen Hemminger <redacted>
Date: Tue, 13 Sep 2005 09:26:59 -0700

Since when is bringing a network device up/down performance critical?

The issue is the dev_get()'s that occur all over the place
to during packet transmit/receive, that's what they are
trying to address.

I'm still against all of these invasive NUMA changes to the
networking though, they are simply too ugly and special cased
to consider seriously.

On Tue, Sep 13, 2005 at 01:26:07PM -0700, David S. Miller wrote:

From: Stephen Hemminger <redacted>
Date: Tue, 13 Sep 2005 09:26:59 -0700

quoted

Since when is bringing a network device up/down performance critical?

The issue is the dev_get()'s that occur all over the place
to during packet transmit/receive, that's what they are
trying to address.

I'm still against all of these invasive NUMA changes to the
networking though, they are simply too ugly and special cased
to consider seriously.

All of them or the dst ones?  Hopefully the netdevice refcounter patch
is not ugly or complicated as the dst ones? And why are they special cased?
Are networking workloads with high route locality not interesting?

Thanks,
Kiran