Extend lock less TX to real devices

PATCH In review Andi Kleen 5 participants 6 messages Aug 31, 2004 – Sep 4, 2004

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

Andi Kleen

author

2004-08-31 12:38 UTC #0 lore raw

PATCH 1/1 Extend lock less TX to real devices

This patch extends the recently added NETIF_F_LLTX to real devices. A lot of modern network drivers have a single big lock around their hard_start_xmit, and it doesn't make sense to take the xmit lock too. They also have enough locking to handle setting of multicast lists properly.

Now they can set NETIF_F_LLTX and get one lock less. For drivers that don't set this flag nothing changes.

I added support for trylocking instead of spinning like sch_generic does - for that the driver has to return -1, then the packet is requeued. The check for a local device deadlock is lost for this case, but that doesn't seem to be a big loss (I've never seen this printk ever get triggered)

The patch looks bigger than it really is because i moved some code around and converted the macros into inlines.

I also did an additional micro optimization: on a preemptive kernel dev_queue_xmit would change the local atomic count several times in the hot path. This patch changes it to disable preemption only once, to hopefully make it a bit faster.

I changed the lltx handling to not change xmit_lock_owner. Without a lock it is not safe anyways and it will prevent another cache line from bouncing.

With only this patch nothing changes because no driver uses the new flag yet.

-Andi

net/core/dev.c renamed from net/core/dev.c-o +35 ~~-27~~

diff -u linux-2.6.8-work/net/core/dev.c-o linux-2.6.8-work/net/core/dev.c
@@ -1255,19 +1255,24 @@
 	return 0;
 }
 
-#define HARD_TX_LOCK_BH(dev, cpu) {			\
-	if ((dev->features & NETIF_F_LLTX) == 0) {	\
-		spin_lock_bh(&dev->xmit_lock);		\
-		dev->xmit_lock_owner = cpu;		\
-	}						\
-}
+/* Preemption must be off here. A driver setting LLTX has to 
+   use interrupt safe locking. */
 
-#define HARD_TX_UNLOCK_BH(dev) {			\
-	if ((dev->features & NETIF_F_LLTX) == 0) {	\
-		dev->xmit_lock_owner = -1;		\
-		spin_unlock_bh(&dev->xmit_lock);	\
-	}						\
-}
+static inline int hard_tx_lock(struct net_device *dev) 
+{ 
+	if (dev->features & NETIF_F_LLTX)
+		return 1; 
+	spin_lock(&dev->xmit_lock);
+	dev->xmit_lock_owner = smp_processor_id(); 
+} 
+
+static inline void hard_tx_unlock(struct net_device *dev) 
+{ 
+	if (dev->features & NETIF_F_LLTX) 
+		return; 
+	dev->xmit_lock_owner = -1;
+	spin_unlock(&dev->xmit_lock);	
+} 
 
 static inline void qdisc_run(struct net_device *dev)
 {

@@ -1319,7 +1324,15 @@
 	      	if (skb_checksum_help(&skb, 0))
 	      		goto out_kfree_skb;
 
-	rcu_read_lock();
+	/* 
+	 * Various locks need a _bh disable and there are CPU local
+	 * data structures too. Instead of changing the preempt count
+	 * with each lock just grab it here once for the whole
+	 * function. This also gives the require atomicity for the RCU
+	 * use below.
+	 */ 
+	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

@@ -1339,18 +1352,17 @@
 #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...

@@ -1363,32 +1375,27 @@
 
 	   Check this and shot the lock. It is not prone from deadlocks.
 	   Either shot noqueue qdisc, it is even simpler 8)
+
+	   BHs are still disabled here
 	 */
 	if (dev->flags & IFF_UP) {
-		int cpu = get_cpu();
-
-		if (dev->xmit_lock_owner != cpu) {
-
-			HARD_TX_LOCK_BH(dev, cpu);
-			put_cpu();
-
+		if (hard_tx_lock(dev)) { 
 			if (!netif_queue_stopped(dev)) {
 				if (netdev_nit)
 					dev_queue_xmit_nit(skb, dev);
 
 				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())

@@ -1401,6 +1408,7 @@
 out_kfree_skb:
 	kfree_skb(skb);
 out:
+	local_bh_enable();
 	return rc;
 }

net/core/pktgen.c renamed from net/core/pktgen.c-o +5 -4

diff -u linux-2.6.8-work/net/core/pktgen.c-o linux-2.6.8-work/net/core/pktgen.c
@@ -629,8 +629,9 @@
 		}
 
 		nr_frags = skb_shinfo(skb)->nr_frags;
-		   
-		spin_lock_bh(&odev->xmit_lock);
+
+		if (!(odev->features & NETIF_F_LLTX))
+			spin_lock_bh(&odev->xmit_lock);
 		if (!netif_queue_stopped(odev)) {
 
 			atomic_inc(&skb->users);

@@ -655,8 +656,8 @@
 			last_ok = 0;
 		}
 		
-
-		spin_unlock_bh(&odev->xmit_lock);
+		if (!(odev->features & NETIF_F_LLTX))
+			spin_unlock_bh(&odev->xmit_lock);
 
 		if (info->ipg) {
 			/* Try not to busy-spin if we have larger sleep times.

net/sched/sch_generic.c renamed from net/sched/sch_generic.c-o +64 ~~-36~~

126 lines not on this page · raw message

David S. Miller

2004-09-02 05:33 UTC #1 lore raw

Quoted 1 line · On Tue, 31 Aug 2004 14:38:20 +0200 Andi Kleen <[email protected]> wrote:

This patch extends the recently added NETIF_F_LLTX to real devices.

Well, it does a lot of other things too.

Quoted 5 lines

I added support for trylocking instead of spinning like sch_generic does - for that the driver has to return -1, then the packet is requeued. The check for a local device deadlock is lost for this case, but that doesn't seem to be a big loss (I've never seen this printk ever get triggered)

It is triggerable if you misconfigure your system. I'm totally against this change, because previously at least the user would find out in their logs. With your change the system explodes looping with no explanation why.

Quoted 2 lines

The patch looks bigger than it really is because i moved some code around and converted the macros into inlines.

Quoted 1 line

I also did an additional micro optimization:

And for this reason you need to split this patch up. I would recommend:

patch 1) Change macros into inlines
patch 2) local_bh_disable() preemption count optimization patch 3) support for F_LLTX on real devices patch 4) locking changes

Thanks Andi.

Andi Kleen

author

2004-09-04 13:28 UTC #2 lore raw

Quoted 6 lines · David S. Miller

Quoted 1 line · On Tue, 31 Aug 2004 14:38:20 +0200 Andi Kleen <[email protected]> wrote:
This patch extends the recently added NETIF_F_LLTX to real devices.
Well, it does a lot of other things too.

Not really, it all works to the same goal.

Quoted 8 lines

Quoted 6 lines
I added support for trylocking instead of spinning like sch_generic does - for that the driver has to return -1, then the packet is requeued. The check for a local device deadlock is lost for this case, but that doesn't seem to be a big loss (I've never seen this printk ever get triggered)
It is triggerable if you misconfigure your system.

Really? The only reason I can see for it is a buggy driver.

Quoted 1 line

I'm totally against this change, because previously at

There is no change, except for drivers that set LLTX and these get different semantics anyways because they have to handle this on their own. In case the driver has bugs I guess it would be better to add the printk directly below the try_lock in the LLTX driver.

Quoted 2 lines

least the user would find out in their logs. With your change the system explodes looping with no explanation why.

Hmm, I guess if you're really worried about this class of driver bugs being common adding some real error handling for it (like bailing out and disabling the device) would be the far better option.

Quoted 6 lines

Quoted 3 lines
The patch looks bigger than it really is because i moved some code around and converted the macros into inlines.
..
Quoted 1 line
I also did an additional micro optimization:

Quoted 7 lines

And for this reason you need to split this patch up. I would recommend:
patch 1) Change macros into inlines
patch 2) local_bh_disable() preemption count optimization patch 3) support for F_LLTX on real devices patch 4) locking changes

At least (3) and (4) are the same thing. I can drop the inlines, it was only for making the code clearer and less ugly but is not essential for the optimizations.

-Andi

jamal

2004-09-04 14:11 UTC #3 lore raw

Quoted 9 lines · Andi Kleen

Quoted 6 lines · David S. Miller
Quoted 1 line · On Tue, 31 Aug 2004 14:38:20 +0200 Andi Kleen <[email protected]> wrote:
This patch extends the recently added NETIF_F_LLTX to real devices.
Well, it does a lot of other things too.
Not really, it all works to the same goal.

Must be my sleep depravation - what is LLTX again?

Quoted 7 lines

Quoted 2 lines
least the user would find out in their logs. With your change the system explodes looping with no explanation why.
Hmm, I guess if you're really worried about this class of driver bugs ble eing common adding some real error handling for it (like bailing out and disabling the device) would be the far better option.

Actually that message is pretty useful.
I have seen at least a handful of badly written drivers do that. Was also very useful for me when i was doing the tc extensions. I was able to catch a few bugs - so not just driver related.

Quoted 8 lines

Quoted 4 lines
patch 1) Change macros into inlines
patch 2) local_bh_disable() preemption count optimization patch 3) support for F_LLTX on real devices patch 4) locking changes
At least (3) and (4) are the same thing. I can drop the inlines, it was only for making the code clearer and less ugly but is not essential for the optimizations.

do you guys mind if i test these patches/patch out first before final inclusion? Next weekend i will have the chance.

cheers,
jamal

Andi Kleen

2004-09-04 14:24 UTC #4 lore raw

Quoted 5 lines · jamal

Quoted 4 lines · Andi Kleen
Quoted 3 lines · David S. Miller
On Tue, 31 Aug 2004 14:38:20 +0200
Andi Kleen <[email protected]> wrote:

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
This patch extends the recently added NETIF_F_LLTX to real devices.
Well, it does a lot of other things too.
Not really, it all works to the same goal.
Must be my sleep depravation - what is LLTX again?

NETIF_F_LLTX - a new flag that tells the stack the the driver doesn't want an xmit lock.

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
least the user would find out in their logs. With your change the system explodes looping with no explanation why.

Quoted 7 lines

Quoted 4 lines
Hmm, I guess if you're really worried about this class of driver bugs ble eing common adding some real error handling for it (like bailing out and disabling the device) would be the far better option.
Actually that message is pretty useful.
I have seen at least a handful of badly written drivers do that.

They will still print that, no problem.

Quoted 6 lines

Quoted 6 lines
Quoted 5 lines
patch 1) Change macros into inlines
patch 2) local_bh_disable() preemption count optimization patch 3) support for F_LLTX on real devices patch 4) locking changes

Quoted 6 lines

Quoted 3 lines
At least (3) and (4) are the same thing. I can drop the inlines, it was only for making the code clearer and less ugly but is not essential for the optimizations.
do you guys mind if i test these patches/patch out first before final inclusion? Next weekend i will have the chance.

You can do that, but they won't do much unless your driver sets NETIF_F_LLTX.

-Andi

Herbert Xu

2004-09-04 19:39 UTC #5 lore raw

In reply to Andi Kleen

Quoted 10 lines · Andi Kleen

Quoted 8 lines
Quoted 6 lines
I added support for trylocking instead of spinning like sch_generic does - for that the driver has to return -1, then the packet is requeued. The check for a local device deadlock is lost for this case, but that doesn't seem to be a big loss (I've never seen this printk ever get triggered)
It is triggerable if you misconfigure your system.
Really? The only reason I can see for it is a buggy driver.

Is this the dead loop message? If so it can happen with tunnels.

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