Skip to contentSkip to conversation

Fix locking bug in lltx path

PATCH In review Andi Kleen 5 participants 6 messages Sep 13, 2004 – Sep 14, 2004

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/1 Fix locking bug in lltx path

Thanks to Arjan's spinlock debug kernel for finding it.

This fixes a silly missing spin lock in the relock path. For some reason it seems to still work when you don't have spinlock debugging enabled.

Please apply.

-Andi

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

Fix missing spin lock in lltx path.

Thanks to Arjan's spinlock debug kernel for finding it.

Signed-off-by Andi Kleen <[email protected]>
net/sched/sch_generic.c renamed from net/sched/sch_generic.c-X +3 -1
diff -u linux-2.6.9rc1-bk19/net/sched/sch_generic.c-X linux-2.6.9rc1-bk19/net/sched/sch_generic.c
@@ -153,8 +153,10 @@
 					spin_lock(&dev->queue_lock);
 					return -1;
 				}
-				if (ret == -1 && nolock)
+				if (ret == -1 && nolock) {
+					spin_lock(&dev->queue_lock);
 					goto collision; 
+				}
 			}
 
 			/* Release the driver */
#1 lore raw
Quoted 7 lines · On Mon, 13 Sep 2004 20:11:00 +0200 Andi Kleen <[email protected]> wrote:

Thanks to Arjan's spinlock debug kernel for finding it.

This fixes a silly missing spin lock in the relock path. For some reason it seems to still work when you don't have spinlock debugging enabled.

Please apply.

Good catch, but I'll need to fix this up to use the new macros defined in linux/netdevice.h

Thanks again.

#2 lore raw
Quoted 5 lines · David S. Miller
Quoted 2 lines · On Mon, 13 Sep 2004 20:11:00 +0200 Andi Kleen <[email protected]> wrote:

Thanks to Arjan's spinlock debug kernel for finding it.

Quoted 8 lines
Quoted 5 lines

This fixes a silly missing spin lock in the relock path. For some reason it seems to still work when you don't have spinlock debugging enabled.

Please apply.

Good catch, but I'll need to fix this up to use the new macros defined in linux/netdevice.h

Ok, do you want me to send a new patch with that?

I don't think the new macros are in -bk* snapshots yet, and getting it otherwise is difficult for me.

It's probably fastest if you just apply it by hand.

-Andi

#3 lore raw
Quoted 10 lines · On Mon, 13 Sep 2004 21:26:49 +0200 Andi Kleen <[email protected]> wrote:
Quoted 9 lines · David S. Miller
Quoted 6 lines · On Mon, 13 Sep 2004 20:11:00 +0200 Andi Kleen <[email protected]> wrote:

Thanks to Arjan's spinlock debug kernel for finding it.

This fixes a silly missing spin lock in the relock path. For some reason it seems to still work when you don't have spinlock debugging enabled.

Quoted 6 lines
Quoted 4 lines
Quoted 1 line

Please apply.

Good catch, but I'll need to fix this up to use the new macros defined in linux/netdevice.h

Ok, do you want me to send a new patch with that?

I'll take care of it.

Quoted 2 lines

I don't think the new macros are in -bk* snapshots yet, and getting it otherwise is difficult for me.

It's in 2.6.9-rc2

#4 lore raw
In reply to Andi Kleen
Quoted 21 lines · Andi Kleen

Thanks to Arjan's spinlock debug kernel for finding it.

This fixes a silly missing spin lock in the relock path. For some reason it seems to still work when you don't have spinlock debugging enabled.

Please apply.

-Andi

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

Fix missing spin lock in lltx path.

Thanks to Arjan's spinlock debug kernel for finding it.

Signed-off-by Andi Kleen <[email protected]>

diff -u linux-2.6.9rc1-bk19/net/sched/sch_generic.c-X linux-2.6.9rc1-bk19/net/sched/sch_generic.c

net/sched/sch_generic.c line 159

Quoted 8 lines
net/sched/sch_generic.c renamed from net/sched/sch_generic.c-X +3 -1
@@ -153,8 +153,10 @@
 					spin_lock(&dev->queue_lock);
 					return -1;
 				}
-				if (ret == -1 && nolock)
+				if (ret == -1 && nolock) {
+					spin_lock(&dev->queue_lock);
 					goto collision; 
+				}

please use the fancy new constants :)

Jeff

#5 lore raw
In reply to Andi Kleen

Finaly got around to testing today and got bitten by this (was planning to send patch);->

In any case, some good news: one of the NICs tested was e1000 with and without LLTX pure forwarding (eth0->eth1 and eth1->eth0, 64 byte packets beating up the interfaces at 1/48Mpps): did not see
much difference between the two. There did seem to be some latency improvements (in the range of 50 microsecs or so for the LLTX but could be experimental error, needs more validation).
I think perhaps a case where multi processors fanning into one NIC would be a better test to see the value of LLTX.
The good news (for me) is it doesnt seem to affect things negatively.

[We saw some corrupt netpoll packets but that could be something else broken].

cheers,
jamal

Quoted 36 lines · Andi Kleen

Thanks to Arjan's spinlock debug kernel for finding it.

This fixes a silly missing spin lock in the relock path. For some reason it seems to still work when you don't have spinlock debugging enabled.

Please apply.

-Andi

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

Fix missing spin lock in lltx path.

Thanks to Arjan's spinlock debug kernel for finding it.

Signed-off-by Andi Kleen <[email protected]>

diff -u linux-2.6.9rc1-bk19/net/sched/sch_generic.c-X linux-2.6.9rc1-bk19/net/sched/sch_generic.c

net/sched/sch_generic.c renamed from net/sched/sch_generic.c-X +3 -1
@@ -153,8 +153,10 @@
 					spin_lock(&dev->queue_lock);
 					return -1;
 				}
-				if (ret == -1 && nolock)
+				if (ret == -1 && nolock) {
+					spin_lock(&dev->queue_lock);
 					goto collision; 
+				}
 			}
 
 			/* Release the driver */