Skip to contentSkip to conversation

Do less atomic count changes in dev_queue_xmit

PATCH Applied Andi Kleen 3 participants 4 messages Sep 4, 2004 – Sep 7, 2004

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

Andi Kleen

author
#0 lore raw

PATCH 1/1 Do less atomic count changes in dev_queue_xmit

Do a single local_bh_disable and a single local_bh_enable instead of changing the atomic count all the time in dev_queue_xmit.

Should mostly benefit preemptible kernels, but others see some small improvements too.

net/core/dev.c renamed from net/core/dev.c-o +15 -13
diff -u linux-2.6.8/net/core/dev.c-o linux-2.6.8/net/core/dev.c
@@ -1249,14 +1249,14 @@
 	return 0;
 }
 
-#define HARD_TX_LOCK_BH(dev, cpu) {			\
+#define HARD_TX_LOCK(dev, cpu) {			\
 	if ((dev->features & NETIF_F_LLTX) == 0) {	\
 		spin_lock_bh(&dev->xmit_lock);		\
 		dev->xmit_lock_owner = cpu;		\
 	}						\
 }
 
-#define HARD_TX_UNLOCK_BH(dev) {			\
+#define HARD_TX_UNLOCK(dev) {				\
 	if ((dev->features & NETIF_F_LLTX) == 0) {	\
 		dev->xmit_lock_owner = -1;		\
 		spin_unlock_bh(&dev->xmit_lock);	\

@@ -1313,7 +1313,12 @@
 	      	if (skb_checksum_help(&skb, 0))
 	      		goto out_kfree_skb;
 
-	rcu_read_lock();
+
+	/* Disable soft irqs for various locks below. Also 
+	 * stops preemption for RCU. 
+	 */
+	local_bh_disable(); 
+
 	/* Updates of qdisc are serialized by queue_lock. 
 	 * The struct Qdisc which is pointed to by qdisc is now a 
 	 * rcu structure - it may be accessed without acquiring 

@@ -1332,18 +1337,16 @@
 #endif
 	if (q->enqueue) {
 		/* Grab device queue */
-		spin_lock_bh(&dev->queue_lock);
+		spin_lock(&dev->queue_lock);
 
 		rc = q->enqueue(skb, q);
 
 		qdisc_run(dev);
 
-		spin_unlock_bh(&dev->queue_lock);
-		rcu_read_unlock();
+		spin_unlock(&dev->queue_lock);
 		rc = rc == NET_XMIT_BYPASS ? NET_XMIT_SUCCESS : rc;
 		goto out;
 	}
-	rcu_read_unlock();
 
 	/* The device has no queue. Common case for software devices:
 	   loopback, all the sorts of tunnels...

@@ -1358,12 +1361,11 @@
 	   Either shot noqueue qdisc, it is even simpler 8)
 	 */
 	if (dev->flags & IFF_UP) {
-		int cpu = get_cpu();
+		int cpu = smp_processor_id(); /* ok because BHs are off */
 
 		if (dev->xmit_lock_owner != cpu) {
 
-			HARD_TX_LOCK_BH(dev, cpu);
-			put_cpu();
+			HARD_TX_LOCK(dev, cpu);
 
 			if (!netif_queue_stopped(dev)) {
 				if (netdev_nit)

@@ -1371,17 +1373,16 @@
 
 				rc = 0;
 				if (!dev->hard_start_xmit(skb, dev)) {
-					HARD_TX_UNLOCK_BH(dev);
+					HARD_TX_UNLOCK(dev);
 					goto out;
 				}
 			}
-			HARD_TX_UNLOCK_BH(dev);
+			HARD_TX_UNLOCK(dev);
 			if (net_ratelimit())
 				printk(KERN_CRIT "Virtual device %s asks to "
 				       "queue packet!\n", dev->name);
 			goto out_enetdown;
 		} else {
-			put_cpu();
 			/* Recursion is detected! It is possible,
 			 * unfortunately */
 			if (net_ratelimit())

@@ -1394,6 +1395,7 @@
 out_kfree_skb:
 	kfree_skb(skb);
 out:
+	local_bh_enable();
 	return rc;
 }
#1 lore raw

net/core/dev.c line 1254

Quoted 12 lines · Andi Kleen

diff -u linux-2.6.8/net/core/dev.c-o linux-2.6.8/net/core/dev.c

net/core/dev.c renamed from net/core/dev.c-o +1 -1
@@ -1249,14 +1249,14 @@
 	return 0;
 }
 
-#define HARD_TX_LOCK_BH(dev, cpu) {			\
+#define HARD_TX_LOCK(dev, cpu) {			\
 	if ((dev->features & NETIF_F_LLTX) == 0) {	\
 		spin_lock_bh(&dev->xmit_lock);		\

You can remove the _bh here as well.

net/core/dev.c line 1364

Quoted 6 lines
net/core/dev.c renamed from net/core/dev.c-o +1 -1
@@ -1358,12 +1361,11 @@
 	   Either shot noqueue qdisc, it is even simpler 8)
 	 */
 	if (dev->flags & IFF_UP) {
-		int cpu = get_cpu();
+		int cpu = smp_processor_id(); /* ok because BHs are off */

Hmm this means that the loopback xmit function will now execute with BH/preempt turned off. Is this what we want?

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#2 lore raw
Quoted 9 lines · Herbert Xu
Quoted 8 lines · Andi Kleen

diff -u linux-2.6.8/net/core/dev.c-o linux-2.6.8/net/core/dev.c

net/core/dev.c renamed from net/core/dev.c-o +0 -0
@@ -1249,14 +1249,14 @@
 	return 0;
 }
Quoted 6 lines

net/core/dev.c line 1254

Quoted 4 lines
net/core/dev.c renamed from net/core/dev.c-o +1 -1
-#define HARD_TX_LOCK_BH(dev, cpu) {			\
+#define HARD_TX_LOCK(dev, cpu) {			\
 	if ((dev->features & NETIF_F_LLTX) == 0) {	\
 		spin_lock_bh(&dev->xmit_lock);		\

You can remove the _bh here as well.

Ok done.

Quoted 10 lines

net/core/dev.c line 1364

Quoted 7 lines
net/core/dev.c renamed from net/core/dev.c-o +1 -1
@@ -1358,12 +1361,11 @@
 	   Either shot noqueue qdisc, it is even simpler 8)
 	 */
 	if (dev->flags & IFF_UP) {
-		int cpu = get_cpu();
+		int cpu = smp_processor_id(); /* ok because BHs are off */

Hmm this means that the loopback xmit function will now execute with BH/preempt turned off. Is this what we want?

I think so yes. It is not that costly.

David, can you please consider this patch, thanks?

-Andi

---------------------------------------------------------------

Streamline atomic count handling in queue xmit fast path. Only do it once instead of multiple times.

net/core/dev.c renamed from net/core/dev.c-o +17 -15
diff -u linux-2.6.8/net/core/dev.c-o linux-2.6.8/net/core/dev.c
@@ -1249,17 +1249,17 @@
 	return 0;
 }
 
-#define HARD_TX_LOCK_BH(dev, cpu) {			\
+#define HARD_TX_LOCK(dev, cpu) {			\
 	if ((dev->features & NETIF_F_LLTX) == 0) {	\
-		spin_lock_bh(&dev->xmit_lock);		\
+		spin_lock(&dev->xmit_lock);		\
 		dev->xmit_lock_owner = cpu;		\
 	}						\
 }
 
-#define HARD_TX_UNLOCK_BH(dev) {			\
+#define HARD_TX_UNLOCK(dev) {				\
 	if ((dev->features & NETIF_F_LLTX) == 0) {	\
 		dev->xmit_lock_owner = -1;		\
-		spin_unlock_bh(&dev->xmit_lock);	\
+		spin_unlock(&dev->xmit_lock);		\
 	}						\
 }
 

@@ -1313,7 +1313,12 @@
 	      	if (skb_checksum_help(&skb, 0))
 	      		goto out_kfree_skb;
 
-	rcu_read_lock();
+
+	/* Disable soft irqs for various locks below. Also 
+	 * stops preemption for RCU. 
+	 */
+	local_bh_disable(); 
+
 	/* Updates of qdisc are serialized by queue_lock. 
 	 * The struct Qdisc which is pointed to by qdisc is now a 
 	 * rcu structure - it may be accessed without acquiring 

@@ -1332,18 +1337,16 @@
 #endif
 	if (q->enqueue) {
 		/* Grab device queue */
-		spin_lock_bh(&dev->queue_lock);
+		spin_lock(&dev->queue_lock);
 
 		rc = q->enqueue(skb, q);
 
 		qdisc_run(dev);
 
-		spin_unlock_bh(&dev->queue_lock);
-		rcu_read_unlock();
+		spin_unlock(&dev->queue_lock);
 		rc = rc == NET_XMIT_BYPASS ? NET_XMIT_SUCCESS : rc;
 		goto out;
 	}
-	rcu_read_unlock();
 
 	/* The device has no queue. Common case for software devices:
 	   loopback, all the sorts of tunnels...

@@ -1358,12 +1361,11 @@
 	   Either shot noqueue qdisc, it is even simpler 8)
 	 */
 	if (dev->flags & IFF_UP) {
-		int cpu = get_cpu();
+		int cpu = smp_processor_id(); /* ok because BHs are off */
 
 		if (dev->xmit_lock_owner != cpu) {
 
-			HARD_TX_LOCK_BH(dev, cpu);
-			put_cpu();
+			HARD_TX_LOCK(dev, cpu);
 
 			if (!netif_queue_stopped(dev)) {
 				if (netdev_nit)

@@ -1371,17 +1373,16 @@
 
 				rc = 0;
 				if (!dev->hard_start_xmit(skb, dev)) {
-					HARD_TX_UNLOCK_BH(dev);
+					HARD_TX_UNLOCK(dev);
 					goto out;
 				}
 			}
-			HARD_TX_UNLOCK_BH(dev);
+			HARD_TX_UNLOCK(dev);
 			if (net_ratelimit())
 				printk(KERN_CRIT "Virtual device %s asks to "
 				       "queue packet!\n", dev->name);
 			goto out_enetdown;
 		} else {
-			put_cpu();
 			/* Recursion is detected! It is possible,
 			 * unfortunately */
 			if (net_ratelimit())

@@ -1394,6 +1395,7 @@
 out_kfree_skb:
 	kfree_skb(skb);
 out:
+	local_bh_enable();
 	return rc;
 }
maintainer Applied
#3 lore raw
Quoted 1 line · On Tue, 7 Sep 2004 13:56:55 +0200 Andi Kleen <[email protected]> wrote:

David, can you please consider this patch, thanks?

Applied, thanks guys.

Andi can you please start providing "Signed-off-by:" lines with your patches? I can only add my own at this time, but it makes me feel better if you provide one too.

Thanks.