LLTX and netif_stop_queue
Subsystems (6)
While testing my IP-over-InfiniBand driver, I discovered that if a net device sets NETIF_F_LLTX, it seems the device's hard_start_xmit method can be called even after a netif_stop_queue().
This is because in the LLTX case, qdisc_restart() holds no locks while calling hard_start_xmit, so something like the following can happen:
CPU 1 CPU 2
qdisc_restart:
drop queue lock
call hard_start_xmit()
net driver:
acquire TX lock
queue packet to HW
acquire queue lock... qdisc_restart:
drop queue lock call hard_start_xmit:
queue full, call netif_stop_queue()
release TX lock
net driver:
acquire TX lock queue is already full!
Is my understanding correct? If so it seems the patch below would make sense. (e1000 seems to handle this properly already)
Thanks,
Roland
Since tg3 and sungem now use lockless TX (NETIF_F_LLTX), it's possible for their hard_start_xmit method to be called even after they call netif_stop_queue. Therefore a full queue no longer indicates a bug -- this patch fixes the comment and removes the KERN_ERR printk.
drivers/net/sungem.c +1 -3
@@ -976,12 +976,10 @@return NETDEV_TX_LOCKED ; } -/* This is a hard error, log it. */ +/* This may happen, since we have NETIF_F_LLTX set */ if (TX_BUFFS_AVAIL (gp) <= (skb_shinfo (skb)->nr_frags +1 )) {netif_stop_queue (dev);spin_unlock_irqrestore (&gp->tx_lock, flags); - printk(KERN_ERR PFX "%s: BUG! Tx Ring full when queue awake!\n", - dev->name);return NETDEV_TX_BUSY ; }
drivers/net/tg3.c +1 -3
@@ -3076,12 +3076,10 @@return NETDEV_TX_LOCKED ; } -/* This is a hard error, log it. */ +/* This may happen, since we have NETIF_F_LLTX set */ if (unlikely (TX_BUFFS_AVAIL (tp) <= (skb_shinfo (skb)->nr_frags +1 ))) {netif_stop_queue (dev);spin_unlock_irqrestore (&tp->tx_lock, flags); - printk(KERN_ERR PFX "%s: BUG! Tx Ring full when queue awake!\n", - dev->name);return NETDEV_TX_BUSY ; }
David S. Miller
maintainerQuoted 6 lines · On Fri, 17 Dec 2004 13:57:40 -0800 Roland Dreier <[email protected]> wrote:
While testing my IP-over-InfiniBand driver, I discovered that if a net device sets
NETIF_F_LLTX, it seems the device'shard_start_xmitmethod can be called even after anetif_stop_queue().This is because in the LLTX case,
qdisc_restart()holds no locks while callinghard_start_xmit, so something like the following can happen:
...
drivers/net/sungem.c line 984
Quoted 7 lines
drivers/net/sungem.c +0
-2if (TX_BUFFS_AVAIL (gp) <= (skb_shinfo (skb)->nr_frags +1 )) {netif_stop_queue (dev);spin_unlock_irqrestore (&gp->tx_lock, flags); - printk(KERN_ERR PFX "%s: BUG! Tx Ring full when queue awake!\n", - dev->name);return NETDEV_TX_BUSY ; }
I understand the bug, but we're not going to fix it this way. This is a crucial invariant that we need to check for because it indicates a pretty serious state error except in this bug case you've discovered.
Perhaps one way to fix this is to add a pointer to a spinlock to the netdev struct, and have hold that the upper level grab that when NETIF_F_LLTX when doing queue state checks. Actually, that could end up being racy too.
If we can't find a good fix for this, besides removing the necessary debugging message, we might have to pull NETIF_F_LLTX out or disable it temporarily until we figure out a way.
David> I understand the bug, but we're not going to fix it this David> way. This is a crucial invariant that we need to check for David> because it indicates a pretty serious state error except in David> this bug case you've discovered.
OK, that's fair enough. I was pretty bummed myself when I realized hard_start_xmit was getting called even after I stopped the queue. Thanks for confirming my analysis.
David> Perhaps one way to fix this is to add a pointer to a David> spinlock to the netdev struct, and have hold that the upper David> level grab that when NETIF_F_LLTX when doing queue state David> checks. Actually, that could end up being racy too.
I may be missing something, but it seems to me that we get all of the benefits of LLTX by just documenting that device drivers can use the xmit_lock member of struct net_device to synchronize other parts of the driver against hard_start_xmit. I guess the driver also should set xmit_lock_owner to -1 after it acquires xmit_lock.
This would mean that NETIF_F_LLTX can go away, and LLTX drivers would just replace their use of their own private tx_lock with xmit_lock (except in hard_start_xmit, where the upper layer already holds xmit_lock).
By the way, am I correct in thinking that the use of xmit_lock_owner in qdisc_restart() is racy?
if (!spin_trylock(&dev->xmit_lock)) {
/* get the lock */
if (!spin_trylock(&dev->xmit_lock)) {
/* fail */
if (dev->xmit_lock_owner == smp_processor_id()) {
/* test the wrong value */
/* set the value too late */
dev->xmit_lock_owner = smp_processor_id();I don't see a simple way to fix this unfortunately.
Thanks,
Roland
Roland> I may be missing something, but it seems to me that we get Roland> all of the benefits of LLTX by just documenting that Roland> device drivers can use the xmit_lock member of struct Roland> net_device to synchronize other parts of the driver Roland> against hard_start_xmit. I guess the driver also should Roland> set xmit_lock_owner to -1 after it acquires xmit_lock.
Thinking about this a little more, I realize that there's no reason for the driver to set xmit_lock_owner -- if the driver is able to acquire the lock, then xmit_lock_owner will already be -1.
So it seems LLTX can be replaced by just having drivers use net_device.xmit_lock instead of their own private tx_lock. Assuming this works (and I don't see anything wrong with it) then this seems like a pretty nice solution: we remove some code from the networking core and get rid of all the "trylock" logic in driver's hard_start_xmit.
- Roland
Roland> So it seems LLTX can be replaced by just having drivers Roland> use net_device.xmit_lock instead of their own private Roland> tx_lock. Assuming this works (and I don't see anything Roland> wrong with it) then this seems like a pretty nice Roland> solution: we remove some code from the networking core and Roland> get rid of all the "trylock" logic in driver's Roland> hard_start_xmit.
Actually trying it instead of talking out of my ass...
Just doing this naively without changing the net core can deadlock because the net core acquires dev->xmit_lock without disabling interrupts. So if the driver tries to use xmit_lock in its interrupt handler, it will deadlock if the interrupt occurred during hard_start_xmit. Even doing local_irq_save() in hard_start_xmit isn't good enough, because there's still a window between the net core's call to hard_start_xmit and the actual local_irq_save where xmit_lock is held with interrupts on.
Maybe it makes sense to change NETIF_F_LLTX to NETIF_F_TX_IRQ_DIS or something like that and have that flag mean "disable interrupts when calling hard_start_xmit." (We could just do this unconditionally but I'm not sure if any drivers rely on having interrupts enabled during hard_start_xmit and I'm worried about making a change in semantics like that -- not to mention some drivers may not need interrupts disabled and may not want the cost).
- Roland
Quoted 2 lines · Roland Dreier
By the way, am I correct in thinking that the use of
xmit_lock_ownerinqdisc_restart()is racy?
No.
Quoted 11 lines
if (!spin_trylock(&dev->xmit_lock)) {
/* get the lock */
if (!spin_trylock(&dev->xmit_lock)) {
/* fail */
if (dev->xmit_lock_owner == smp_processor_id()) {
/* test the wrong value */
/* set the value too late */
dev->xmit_lock_owner = smp_processor_id();The setting is protected by the queue lock. So no race.
cheers,
jamal
Quoted 4 lines · David S. Miller
Perhaps one way to fix this is to add a pointer to a spinlock to the netdev struct, and have hold that the upper level grab that when
NETIF_F_LLTXwhen doing queue state checks. Actually, that could end up being racy too.
How about releasing the qlock only when the LLTX transmit lock is grabbed? That should bring it to par with what it was originally.
cheers,
jamal
Quoted 2 lines · jamal
How about releasing the qlock only when the LLTX transmit lock is grabbed? That should bring it to par with what it was originally.
Something like two attached patches... one showing how to do it for e1000. untested (not even compiled)
cheers,
jamal
net/sched/sch_generic.c renamed from net/sched/bak.sch_generic.c +3 -5
@@ -128,12 +128,11 @@ }/* Remember that the driver is grabbed by us. */ dev->xmit_lock_owner =smp_processor_id (); +/* And release queue */ +spin_unlock (&dev->queue_lock); } { -/* And release queue */ -spin_unlock (&dev->queue_lock); -if (!netif_queue_stopped (dev)) {int ret;if (netdev_nit)
@@ -141,15 +140,14 @@ ret = dev->hard_start_xmit (skb, dev);if (ret ==NETDEV_TX_OK ) { + dev->xmit_lock_owner =-1 ;if (!nolock) { - dev->xmit_lock_owner =-1 ;spin_unlock (&dev->xmit_lock); }spin_lock (&dev->queue_lock);return -1 ; }if (ret ==NETDEV_TX_LOCKED && nolock) { -spin_lock (&dev->queue_lock); goto collision; } }
drivers/net/e1000/e1000_main.c renamed from drivers/net/e1000/b.e1000_main.c +4 -0
@@ -1809,6 +1809,10 @@return NETDEV_TX_LOCKED ; } +/* new from sch_generic for LLTX */ +spin_unlock (&dev->queue_lock); + dev->xmit_lock_owner =smp_processor_id (); +/* need: count + 2 desc gap to keep tail from touching * head, otherwise try next time */ if (E1000_DESC_UNUSED (&adapter->tx_ring) < count +2 ) {
Quoted 7 lines · jamal
Quoted 2 lines · jamal
How about releasing the qlock only when the LLTX transmit lock is grabbed? That should bring it to par with what it was originally.
Something like two attached patches... one showing how to do it for e1000. untested (not even compiled)
After attempting to compile, p2 take2.
cheers,
jamal
drivers/net/e1000/e1000_main.c renamed from drivers/net/e1000/bak.e1000_main.c +4 -0
@@ -1809,6 +1809,10 @@return NETDEV_TX_LOCKED ; } +/* new from sch_generic for LLTX */ +spin_unlock (&netdev->queue_lock); + netdev->xmit_lock_owner =smp_processor_id (); +/* need: count + 2 desc gap to keep tail from touching * head, otherwise try next time */ if (E1000_DESC_UNUSED (&adapter->tx_ring) < count +2 ) {
jamal> How about releasing the qlock only when the LLTX transmit jamal> lock is grabbed? That should bring it to par with what it jamal> was originally.
This seems a little risky. I can't point to a specific deadlock but it doesn't seem right on general principles to unlock in a different order than you nested the locks when acquiring them -- if I understand correctly, you're suggesting lock(queue_lock), lock(tx_lock), unlock(queue_lock), unlock(tx_lock).
Thanks,
Roland
Quoted 9 lines · Roland Dreier
jamal> How about releasing the qlock only when the LLTX transmit jamal> lock is grabbed? That should bring it to par with what it jamal> was originally.
This seems a little risky. I can't point to a specific deadlock but it doesn't seem right on general principles to unlock in a different order than you nested the locks when acquiring them -- if I understand correctly, you're suggesting
lock(queue_lock),lock(tx_lock),unlock(queue_lock),unlock(tx_lock).
There is no deadlock. Thats exactly how things work. Try the patches i posted.
cheers,
jamal
Roland> This seems a little risky. I can't point to a specific Roland> deadlock but it doesn't seem right on general principles Roland> to unlock in a different order than you nested the locks Roland> when acquiring them -- if I understand correctly, you're Roland> suggesting lock(queue_lock), lock(tx_lock), Roland> unlock(queue_lock), unlock(tx_lock).
jamal> There is no deadlock. Thats exactly how things work. Try jamal> the patches i posted.
Thinking about it more, I guess it's OK. I was just think about the basic general rule that locks need to be acquired/released in LIFO order to avoid deadlock (eg lock(A), lock(B), unlock(B), unlock(A)).
However in this case unlocking queue_lock after acquiring the driver's tx_lock seems to be OK because the driver does a trylock on tx_lock in the xmit path, so it can't deadlock. At worst the trylock will just fail to get the lock.
Thanks,
Roland
Quoted 9 lines · jamal
Quoted 4 lines · David S. Miller
Perhaps one way to fix this is to add a pointer to a spinlock to the netdev struct, and have hold that the upper level grab that when
NETIF_F_LLTXwhen doing queue state checks. Actually, that could end up being racy too.How about releasing the qlock only when the LLTX transmit lock is grabbed? That should bring it to par with what it was originally.
I dont like the idea of releasing inside the driver a lock taken outside. That might be just me...
Instead, I would suggest to have LLTX drivers check whether queue is stopped after they grab their private tx lock and before they check tx ring fullness. That way we close the race window but keep the driver bug check around.
See attached sungem patch.
Signature · 1 line
Eric
===== drivers/net/sungem.c 1.71 vs edited =====
drivers/net/sungem.c +6 -0
@@ -976,6 +976,12 @@return NETDEV_TX_LOCKED ; } +/* This handles a LLTX-related race condition */ +if (netif_queue_stopped (dev)) { +spin_unlock_irqrestore (&gp->tx_lock, flags); +return NETDEV_TX_BUSY ; + } +/* This is a hard error, log it. */ if (TX_BUFFS_AVAIL (gp) <= (skb_shinfo (skb)->nr_frags +1 )) {netif_stop_queue (dev);
David S. Miller
maintainerQuoted 6 lines · On Wed, 22 Dec 2004 19:49:48 +0100 Eric Lemoine <[email protected]> wrote:
Instead, I would suggest to have LLTX drivers check whether queue is stopped after they grab their private tx lock and before they check tx ring fullness. That way we close the race window but keep the driver bug check around.
See attached sungem patch.
That sounds about right. Nice idea. It solves the race, and retains the error state check.
I'll apply Eric's patch, and do something similar in the other LLTX drivers (except loopback which has not "queue" per se so doesn't need this stuff).
David> That sounds about right. Nice idea. It solves the race, David> and retains the error state check.
Great, I've made a similar change to the IP-over-IB driver.
Thanks,
Roland
Quoted 8 lines · David S. Miller
Quoted 5 lines · On Wed, 22 Dec 2004 19:49:48 +0100 Eric Lemoine <[email protected]> wrote:
Instead, I would suggest to have LLTX drivers check whether queue is stopped after they grab their private tx lock and before they check tx ring fullness. That way we close the race window but keep the driver bug check around.
Quoted 8 lines
Quoted 1 line
See attached sungem patch.
That sounds about right. Nice idea. It solves the race, and retains the error state check.
I'll apply Eric's patch, and do something similar in the other LLTX drivers (except loopback which has not "queue" per se so doesn't need this stuff).
Dave,
I still have one concern with the LLTX code (and it may be that the correct patch is Jamal's) :
Without LLTX we do : lock(queue_lock), lock(xmit_lock), release(queue_lock), release(xmit_lock). With LLTX (without Jamal's patch) we do : lock(queue_lock), release(queue_lock), lock(tx_lock), release(tx_lock). LLTX doesn't look correct because it creates a race condition window between the the two lock-protected sections. So you may want to reconsider Jamal's patch or pull out LLTX...
Thanks,
Signature · 1 line
Eric
Quoted 9 lines · Eric Lemoine
I still have one concern with the LLTX code (and it may be that the correct patch is Jamal's) :
Without LLTX we do :
lock(queue_lock),lock(xmit_lock),release(queue_lock),release(xmit_lock). With LLTX (without Jamal's patch) we do :lock(queue_lock),release(queue_lock),lock(tx_lock),release(tx_lock). LLTX doesn't look correct because it creates a race condition window between the the two lock-protected sections. So you may want to reconsider Jamal's patch or pull out LLTX...
You're right, it can cause packet reordering if something like this happens:
CPU1 CPU2lock(queue_lock)
dequeueunlock(queue_lock)
lock(queue_lock)
dequeue
unlock(queue_lock)
lock(xmit_lock)
hard_start_xmit
unlock(xmit_lock)lock(xmit_lock)hard_start_xmitunlock(xmit_lock)
Jamal's patch should fix this.
Regards
Patrick
Quoted 4 lines · Patrick McHardy
Quoted 3 lines · Eric Lemoine
I still have one concern with the LLTX code (and it may be that the correct patch is Jamal's) :
Quoted 8 lines
Quoted 6 lines
Without LLTX we do :
lock(queue_lock),lock(xmit_lock),release(queue_lock),release(xmit_lock). With LLTX (without Jamal's patch) we do :lock(queue_lock),release(queue_lock),lock(tx_lock),release(tx_lock). LLTX doesn't look correct because it creates a race condition window between the the two lock-protected sections. So you may want to reconsider Jamal's patch or pull out LLTX...You're right, it can cause packet reordering
That's exactly what I was thinking of.
Quoted 18 lines
if something like this
happens:CPU1 CPU2
lock(queue_lock)
dequeueunlock(queue_lock)
lock(queue_lock)
dequeue
unlock(queue_lock)
lock(xmit_lock)
hard_start_xmit
unlock(xmit_lock)lock(xmit_lock)hard_start_xmitunlock(xmit_lock)Jamal's patch should fix this.
Signature · 1 line
Eric
Quoted 28 lines · Patrick McHardy
Quoted 9 lines · Eric Lemoine
I still have one concern with the LLTX code (and it may be that the correct patch is Jamal's) :
Without LLTX we do :
lock(queue_lock),lock(xmit_lock),release(queue_lock),release(xmit_lock). With LLTX (without Jamal's patch) we do :lock(queue_lock),release(queue_lock),lock(tx_lock),release(tx_lock). LLTX doesn't look correct because it creates a race condition window between the the two lock-protected sections. So you may want to reconsider Jamal's patch or pull out LLTX...You're right, it can cause packet reordering if something like this happens:
CPU1 CPU2
lock(queue_lock)
dequeueunlock(queue_lock)
lock(queue_lock)
dequeue
unlock(queue_lock)
lock(xmit_lock)
hard_start_xmit
unlock(xmit_lock)lock(xmit_lock)hard_start_xmitunlock(xmit_lock)
Quoted 1 line
Jamal's patch should fix this.
Yes but requiring drivers to release a lock that they should not even be aware of doesn't sound good. Another way would be to keep dev->queue_lock grabbed when entering start_xmit() and let the driver drop it (and re-acquire it before it returns) only if it wishes so. Although I don't like this too much either, that's the best way I can think of up to now...
Signature · 1 line
Eric
Quoted 6 lines · Eric Lemoine
Yes but requiring drivers to release a lock that they should not even be aware of doesn't sound good. Another way would be to keep
dev->queue_lockgrabbed when enteringstart_xmit()and let the driver drop it (and re-acquire it before it returns) only if it wishes so. Although I don't like this too much either, that's the best way I can think of up to now...
I am not a big fan of that patch either, but i cant think of a cleaner way to do it.
The violation already happens with the LLTX flag. So maybe a big warning that says "Do this only if you driver is LLTX enabled". The other way to do it is put a check to see if LLTX is enabled before releasing that lock - but why the extra cycles? Driver writer should know.
cheers,
jamal
Quoted 9 lines · jamal
Quoted 6 lines · Eric Lemoine
Yes but requiring drivers to release a lock that they should not even be aware of doesn't sound good. Another way would be to keep
dev->queue_lockgrabbed when enteringstart_xmit()and let the driver drop it (and re-acquire it before it returns) only if it wishes so. Although I don't like this too much either, that's the best way I can think of up to now...
Quoted 6 lines
I am not a big fan of that patch either, but i cant think of a cleaner way to do it.
The violation already happens with the LLTX flag. So maybe a big warning that says "Do this only if you driver is LLTX enabled". The other way to do it is put a check to see if LLTX is enabled before releasing that lock - but why the extra cycles? Driver writer should know.
Two (untested) patches implementing what I described above.
The first pach (sch_generic.patch) keeps queue_lock held in qdisc_restart() when calling hard_start_xmit() of a LLTX driver. The second (sungem.patch) makes sungem release queue_lock after grabbing its private tx lock.
Note that the modifications made to qdisc_restart() are not compatible with the current LLTX drivers. All LLTX drivers must be modified along sungem.patch's lines. Take sungem.patch as an example of how things must be done.
Patches apply to current davem bk snapshot.
Signature · 1 line
Eric
===== net/sched/sch_generic.c 1.32 vs edited =====
net/sched/sch_generic.c +16 -10
@@ -104,9 +104,17 @@* locking again. These checks are worth it because * even uncongested locks can be quite expensive. * The driver can do trylock like here too, in case -* of lock congestion it should return -1 and the packet -* will be requeued. +* of lock congestion it should return NETDEV_TX_LOCKED +* and the packet will be requeued. +* +* Note: when the driver has LLTX set queue_lock cannot +* be dropped in here if we want to avoid having packets +* passing eachover during transmit. queue_lock can only +* be dropped in hard_start_xmit() after private tx lock +* has been grabbed. If hard_start_xmit() does release +* queue_lock it must grab it again before it returns. */ +if (!nolock) {if (!spin_trylock (&dev->xmit_lock)) { collision:
@@ -128,12 +136,12 @@ }/* Remember that the driver is grabbed by us. */ dev->xmit_lock_owner =smp_processor_id (); + +/* And release queue */ +spin_unlock (&dev->queue_lock); } { -/* And release queue */ -spin_unlock (&dev->queue_lock); -if (!netif_queue_stopped (dev)) {int ret;if (netdev_nit)
@@ -144,14 +152,12 @@if (!nolock) { dev->xmit_lock_owner =-1 ;spin_unlock (&dev->xmit_lock); +spin_lock (&dev->queue_lock); } -spin_lock (&dev->queue_lock);return -1 ; } -if (ret ==NETDEV_TX_LOCKED && nolock) { -spin_lock (&dev->queue_lock); +if (ret ==NETDEV_TX_LOCKED && nolock) goto collision; - } }/* NETDEV_TX_BUSY - we need to requeue */
@@ -159,8 +165,8 @@if (!nolock) { dev->xmit_lock_owner =-1 ;spin_unlock (&dev->xmit_lock); +spin_lock (&dev->queue_lock); } -spin_lock (&dev->queue_lock); q = dev->qdisc; }
===== drivers/net/sungem.c 1.71 vs edited =====
drivers/net/sungem.c +10 -0
@@ -950,6 +950,7 @@return 0 ; } +/* Called with dev->queue_lock held */ static int gem_start_xmit (struct sk_buff *skb,struct net_device *dev) {struct gem *gp = dev->priv;
@@ -976,8 +977,16 @@return NETDEV_TX_LOCKED ; } +/* We can now drop queue_lock since tx_lock protects us. But remember +* that queue_lock must be reacquired before we return. Moreover we +* must reacquire it before releasing tx_lock to avoid being called +* with the queue stopped. +*/ +spin_unlock (&dev->queue_lock); +/* This is a hard error, log it. */ if (TX_BUFFS_AVAIL (gp) <= (skb_shinfo (skb)->nr_frags +1 )) { +spin_lock (&dev->queue_lock);netif_stop_queue (dev);spin_unlock_irqrestore (&gp->tx_lock, flags); printk(KERN_ERR PFX "%s: BUG! Tx Ring full when queue awake!\n",
@@ -1066,6 +1075,7 @@ dev->name, entry, skb->len);mb ();writel (gp->tx_new, gp->regs +TXDMA_KICK ); +spin_lock (&dev->queue_lock);spin_unlock_irqrestore (&gp->tx_lock, flags); dev->trans_start = jiffies;
Quoted 11 lines
Two (untested) patches implementing what I described above.
The first pach (sch_generic.patch) keeps
queue_lockheld inqdisc_restart()when callinghard_start_xmit()of a LLTX driver. The second (sungem.patch) makes sungem releasequeue_lockafter grabbing its private tx lock.Note that the modifications made to
qdisc_restart()are not compatible with the current LLTX drivers. All LLTX drivers must be modified along sungem.patch's lines. Take sungem.patch as an example of how things must be done.
Correction: the current LLTX drivers need not be modified to work correctly. However, as an opimisation, they can modified along sungem.patch's line.
Thanks,
Signature · 1 line
Eric
this piece:
-----
+
+ /* And release queue */
+ spin_unlock(&dev->queue_lock);
}---------
Isnt there a possibility (higher as you increase number of processors) that you will spin for a long time in _the driver_ waiting for the queue lock?
Maybe we need a flag "the queue lock" is already been grabbed passed to the driver. LLTX is a good alias but insufficient (if you run your driver in an old kernels which support LLTX but not the idea of locking the queue for you).
Andi or anyone else - has anyone really done perfomance analysis of LLTX (with anything other than loopback) and shown it has any value? Maybe time to just shoot the damn thing.
cheers,
jamal
Quoted 30 lines · Eric Lemoine
Quoted 9 lines · jamal
Quoted 6 lines · Eric Lemoine
Yes but requiring drivers to release a lock that they should not even be aware of doesn't sound good. Another way would be to keep
dev->queue_lockgrabbed when enteringstart_xmit()and let the driver drop it (and re-acquire it before it returns) only if it wishes so. Although I don't like this too much either, that's the best way I can think of up to now...Quoted 6 lines
I am not a big fan of that patch either, but i cant think of a cleaner way to do it.
The violation already happens with the LLTX flag. So maybe a big warning that says "Do this only if you driver is LLTX enabled". The other way to do it is put a check to see if LLTX is enabled before releasing that lock - but why the extra cycles? Driver writer should know.Two (untested) patches implementing what I described above.
The first pach (sch_generic.patch) keeps
queue_lockheld inqdisc_restart()when callinghard_start_xmit()of a LLTX driver. The second (sungem.patch) makes sungem releasequeue_lockafter grabbing its private tx lock.Note that the modifications made to
qdisc_restart()are not compatible with the current LLTX drivers. All LLTX drivers must be modified along sungem.patch's lines. Take sungem.patch as an example of how things must be done.Patches apply to current davem bk snapshot.
Quoted 11 lines · jamal
this piece:
-----net/sched/sch_generic.c +3
-1+ +/* And release queue */ +spin_unlock (&dev->queue_lock); } ---------Isnt there a possibility (higher as you increase number of processors) that you will spin for a long time in
_thedriver_waiting for the queue lock?
I don't see how LLTX is different from non-LLTX wrt the time spent spinning on queue_lock. What am i missing?
Quoted 9 lines
Maybe we need a flag "the queue lock" is already been grabbed passed to the driver. LLTX is a good alias but insufficient (if you run your driver in an old kernels which support LLTX but not the idea of locking the queue for you).
Andi or anyone else - has anyone really done perfomance analysis of LLTX (with anything other than loopback) and shown it has any value? Maybe time to just shoot the damn thing.
I do not like LLTX too much either as I can't see a cleaner way to get rid of that packet reordering race condition.
Signature · 1 line
Eric
jamal> Andi or anyone else - has anyone really done perfomance jamal> analysis of LLTX (with anything other than loopback) and jamal> shown it has any value? Maybe time to just shoot the damn jamal> thing.
For the IPoIB driver, it seems to be worth a couple percent. It's hard to get really consistent results but on my pair of test systems, TCP throughput with netperf goes up by about 30 Mbps out of roughly 1900 Mbps total. It's probably not worth the complexity in the net core if all drivers only get that level of improvement.
- Roland
Quoted 5 lines · Roland Dreier
jamal> Andi or anyone else - has anyone really done perfomance jamal> analysis of LLTX (with anything other than loopback) and jamal> shown it has any value? Maybe time to just shoot the damn jamal> thing.
Quoted 8 lines
For the IPoIB driver, it seems to be worth a couple percent. It's hard to get really consistent results but on my pair of test systems, TCP throughput with netperf goes up by about 30 Mbps out of roughly 1900 Mbps total. It's probably not worth the complexity in the net core if all drivers only get that level of improvement.
- Roland
What are your machines? In particular, how many CPUs do they have?
Signature · 1 line
Eric
Eric> What are your machines? In particular, how many CPUs do they have?
Dual Xeons with HT on, so they look like 4 CPUs.
- R.
Quoted 3 lines · Roland Dreier
Eric> What are your machines? In particular, how many CPUs do they have?
Dual Xeons with HT on, so they look like 4 CPUs.
If I understand correctly, LLTX aims at avoiding cache misses on lock variables (because of cacheline bouncing). So the effect of LLTX should increase as the number of CPUs not sharing the same cache increases. And two CPUs might not be enough...
Signature · 1 line
Eric
Quoted 4 lines · Eric Lemoine
If I understand correctly, LLTX aims at avoiding cache misses on lock variables (because of cacheline bouncing). So the effect of LLTX should increase as the number of CPUs not sharing the same cache increases. And two CPUs might not be enough...
Cacheline bouncing usually starts to show up with > 4 CPUs (ie 8 or more). Some workloads that Jamal cares about (routing) only need 2 cpus.
grant
Quoted 1 line · Grant Grundler
Some workloads that Jamal cares about (routing) only need 2 cpus.
What bothers me is more the complexity this has introduced more than the workload. OTOH, 1-2% improvement posted by Roland is good justification.
cheers,
jamal
David S. Miller
maintainerQuoted 7 lines · On 03 Jan 2005 23:18:14 -0500 jamal <[email protected]> wrote:
Quoted 1 line · Grant Grundler
Some workloads that Jamal cares about (routing) only need 2 cpus.
What bothers me is more the complexity this has introduced more than the workload. OTOH, 1-2% improvement posted by Roland is good justification.
I think I'm going to put in something like Eric's patch and fix up the other LLTX drivers as per his sungem patch.
There is a part of me that does want to yank LLTX for non-loopback out of the tree.
Quoted 11 lines · On Wed, 19 Jan 2005 14:47:11 -0800 "David S. Miller" <[email protected]>…
Quoted 7 lines · On 03 Jan 2005 23:18:14 -0500 jamal <[email protected]> wrote:
Quoted 1 line · Grant Grundler
Some workloads that Jamal cares about (routing) only need 2 cpus.
What bothers me is more the complexity this has introduced more than the workload. OTOH, 1-2% improvement posted by Roland is good justification.
Quoted 5 lines
I think I'm going to put in something like Eric's patch and fix up the other LLTX drivers as per his sungem patch.
There is a part of me that does want to yank LLTX for non-loopback out of the tree.
Wondering, why not just have the drivers have a way to lock dev->queue_lock in the interrupt handler, and change the xmit to do spin_lock_irqsave?
Any driver that assumes it is being called with irq's enabled in transmit is probably already busted anyway.
Signature · 1 line
Stephen Hemminger <[email protected]>
David S. Miller
maintainerQuoted 5 lines · On Wed, 19 Jan 2005 15:18:53 -0800 Stephen Hemminger <[email protected]>…
Wondering, why not just have the drivers have a way to lock
dev->queue_lockin the interrupt handler, and change the xmit to dospin_lock_irqsave?Any driver that assumes it is being called with irq's enabled in transmit is probably already busted anyway.
Yes, that's an idea.
Effectively, LLTX is working around the fact that dev->xmit_lock is BH disabling instead of IRQ disabling. And there is no fundamental reason for that.
Usually we strive for BH disabling locks in order to decrease the amount of hard IRQ disabling time in the kernel. But here, as soon as we call into ->hard_start_xmit(), the driver typically turns hard IRQs off anyways.
There are other things using this though, such as multicast list upload.
If we change dev->xmit_lock to be an IRQ disabling lock, then drivers can use it in place of their private tx_lock.
We would still need something special for loopback, which wants and needs no locking at all.
Also, changing dev->xmit_lock to be IRQ disabling is not %100 trivial. We'd need to verify the side-effects this has on gems like the spin_unlock_wait(&dev->xmit_lock) in the Tulip winbond power management code. There is another fun case in the 6pack hamradio driver where it is doing IRQ disabling when taking dev->xmit_lock.
Originally, dev->xmit_lock was added so that drivers that were SMP dumb could stay that way. Thus preserving the guarentee that there would be only one active call into the dev->hard_start_xmit method across the entire system. I don't think any of that is relevant any longer. All of our network drivers are pretty clean in this regard.
Quoted 4 lines · David S. Miller
Usually we strive for BH disabling locks in order to decrease the amount of hard IRQ disabling time in the kernel. But here, as soon as we call into ->hard_start_xmit(), the driver typically turns hard IRQs off anyways.
[...]
Quoted 1 line
Also, changing
dev->xmit_lockto be IRQ disabling is not %100 trivial.
Agreed on both counts...
Quoted 10 lines · On Wed, 19 Jan 2005 19:02:52 -0500 Jeff Garzik <[email protected]> wrote:
Quoted 4 lines · David S. Miller
Usually we strive for BH disabling locks in order to decrease the amount of hard IRQ disabling time in the kernel. But here, as soon as we call into ->hard_start_xmit(), the driver typically turns hard IRQs off anyways.
[...]
Quoted 1 line
Also, changing
dev->xmit_lockto be IRQ disabling is not %100 trivial.Agreed on both counts...
But in the end it would be safer than the current LLTX usage in drivers which seems like it opening up problems.
Signature · 1 line
Stephen Hemminger <[email protected]>
David S. Miller
maintainerQuoted 2 lines · On Wed, 19 Jan 2005 16:46:14 -0800 Stephen Hemminger <[email protected]>…
But in the end it would be safer than the current LLTX usage in drivers which seems like it opening up problems.
Agreed, see my prototype patch in a posting I just made.
David S. Miller <[email protected]> :
[...]
Quoted 5 lines
Originally,
dev->xmit_lockwas added so that drivers that were SMP dumb could stay that way. Thus preserving the guarentee that there would be only one active call into thedev->hard_start_xmitmethod across the entire system. I don't think any of that is relevant any longer. All of our network drivers are pretty clean in this regard.
(nit)
Almost all. I used the fact that dev->hard_start_xmit was issued in a bh disabled context to exchange spinlock_irqsave for ordered ops on ring indexes so as to sync hard_start_xmit and the irq handler in the r8169 driver. It is a bit sick but Jon Mason reported it made a noticeable difference to avoid the irqsave on its 4 way ppc64 and nobody complained about it.
--
Ueimor
David S. Miller
maintainerQuoted 8 lines · On Thu, 20 Jan 2005 01:47:53 +0100 Francois Romieu <[email protected]> wrote:
David S. Miller <[email protected]> :
[...]Quoted 5 lines
Originally,
dev->xmit_lockwas added so that drivers that were SMP dumb could stay that way. Thus preserving the guarentee that there would be only one active call into thedev->hard_start_xmitmethod across the entire system. I don't think any of that is relevant any longer. All of our network drivers are pretty clean in this regard.
Quoted 8 lines
(nit)
Almost all. I used the fact that
dev->hard_start_xmitwas issued in a bh disabled context to exchangespinlock_irqsavefor ordered ops on ring indexes so as to synchard_start_xmitand the irq handler in the r8169 driver. It is a bit sick but Jon Mason reported it made a noticeable difference to avoid the irqsave on its 4 way ppc64 and nobody complained about it.
Hmmm... ok then. Unfortunately, my prototype patch I just posted will make IRQs get disabled in the ->hard_start_xmit() path.
BTW, in your close() routine you do this:
/* Give a racing hard_start_xmit a few cycles to complete. */ set_current_state(TASK_UNINTERRUPTIBLE); schedule_timeout(1);
This is no guarentee of progress. You might instead want to do a synchronize_kernel() or similar, which does in fact guarentee a quiescent state.
Or if my patch goes in use spin_unlock_wait(&netdev->xmit_lock) ;-)
David S. Miller <[email protected]> :
[...]
Quoted 2 lines
Hmmm... ok then. Unfortunately, my prototype patch I just posted will make IRQs get disabled in the ->hard_start_xmit() path.
I'm completely fine with the prototype patch. It will allow to cut some code in the driver and the r8169 has room for improvement anyway.
Point taken for the close() routine.
--
Ueimor
David S. Miller
maintainerPATCH 1/1 : was Re: LLTX and netif_stop_queue
In reply to Stephen HemmingerOk, here is what something like this might look like.
In summary:
1) dev->xmit_lock is now IRQ disabling instead of BH disabling
2) Drivers can use dev->xmit_lock in place of their private driver_priv->tx_lock and this is effectively the same gain LLTX gave drivers sans the race condition which started this thread.
3) NETDEV_TX_LOCKED is gone
4) LLTX stays, but this means that the driver's TX routine is %100 lockless. It is only to be used by loopback and other queueless software devices such as tunnels.
BTW, one next step would be to implement things as Alexey prescribes in net/core/dev.c, that is to make all software devices %100 lockless and use LLTX. Then all the locking code under his comment in dev_queue_xmit() for the queueless device case can be deleted. We can assert the LLTX flag being set in that code path in order to catch unconverted software devices.
Also, this new semantic of LLTX means that it should never be seen set in qdisc_restart() any longer. So we could remove all of that LLTX complexity from that code and assert on it not being set. That would be quite a welcome cleanup.
Comments?
===== Documentation/networking/netdevices.txt 1.5 vs edited =====
Documentation/networking/netdevices.txt +3 -6
@@ -45,10 +45,9 @@ Synchronization: dev->xmit_lock spinlock. When the driver sets NETIF_F_LLTX in dev->features this will be called without holding xmit_lock. In this case the driver - has to lock by itself when needed. It is recommended to use a try lock - for this and return -1 when the spin lock fails. - The locking there should also properly protect against - set_multicast_list + has to execute it's transmission routine in a completely lockless + manner. It is recommended only for queueless devices such + loopback and tunnels. Context: BHs disabled Notes: netif_queue_stopped() is guaranteed false Return codes:
@@ -56,8 +55,6 @@ o NETDEV_TX_BUSY Cannot transmit packet, try later Usually a bug, means queue start/stop flow control is broken in the driver. Note: the driver must NOT put the skb in its DMA ring. - o NETDEV_TX_LOCKED Locking failed, please retry quickly. - Only valid when NETIF_F_LLTX is set. dev->tx_timeout: Synchronization: dev->xmit_lock spinlock.
===== drivers/infiniband/ulp/ipoib/ipoib.h 1.3 vs edited =====
drivers/infiniband/ulp/ipoib/ipoib.h +4 -5
@@ -104,10 +104,10 @@ };/* -* Device private locking: tx_lock protects members used in TX fast -* path (and we use LLTX so upper layers don't do extra locking). -* lock protects everything else. lock nests inside of tx_lock (ie -* tx_lock must be acquired first if needed). +* Device private locking: netdev->xmit_lock protects members used +* in TX fast path. +* lock protects everything else. lock nests inside of xmit_lock (ie +* xmit_lock must be acquired first if needed). */ struct ipoib_dev_priv {spinlock_t lock;
@@ -150,7 +150,6 @@struct ipoib_buf *rx_ring; -spinlock_t tx_lock;struct ipoib_buf *tx_ring;unsigned tx_head;unsigned tx_tail;
===== drivers/infiniband/ulp/ipoib/ipoib_ib.c 1.4 vs edited =====
drivers/infiniband/ulp/ipoib/ipoib_ib.c +2 -2
@@ -247,12 +247,12 @@dev_kfree_skb_any (tx_req->skb); -spin_lock_irqsave (&priv->tx_lock, flags); +spin_lock_irqsave (&dev->xmit_lock, flags); ++priv->tx_tail;if (netif_queue_stopped (dev) && priv->tx_head - priv->tx_tail <=IPOIB_TX_RING_SIZE /2 )netif_wake_queue (dev); -spin_unlock_irqrestore (&priv->tx_lock, flags); +spin_unlock_irqrestore (&dev->xmit_lock, flags);if (wc->status !=IB_WC_SUCCESS && wc->status !=IB_WC_WR_FLUSH_ERR )
===== drivers/infiniband/ulp/ipoib/ipoib_main.c 1.3 vs edited =====
drivers/infiniband/ulp/ipoib/ipoib_main.c +4 -22
@@ -411,7 +411,7 @@/* * We can only be called from ipoib_start_xmit, so we're -* inside tx_lock -- no need to save/restore flags. +* inside dev->xmit_lock -- no need to save/restore flags. */ spin_lock (&priv->lock);
@@ -483,7 +483,7 @@/* * We can only be called from ipoib_start_xmit, so we're -* inside tx_lock -- no need to save/restore flags. +* inside dev->xmit_lock -- no need to save/restore flags. */ spin_lock (&priv->lock);
@@ -526,27 +526,11 @@spin_unlock (&priv->lock); } +/* Called with dev->xmit_lock held and IRQs disabled. */ static int ipoib_start_xmit (struct sk_buff *skb,struct net_device *dev) {struct ipoib_dev_priv *priv =netdev_priv (dev);struct ipoib_neigh *neigh; -unsigned long flags; - -local_irq_save (flags); -if (!spin_trylock (&priv->tx_lock)) { -local_irq_restore (flags); -return NETDEV_TX_LOCKED ; - } - -/* -* Check if our queue is stopped. Since we have the LLTX bit -* set, we can't rely on netif_stop_queue() preventing our -* xmit function from being called with a full queue. -*/ -if (unlikely (netif_queue_stopped (dev))) { -spin_unlock_irqrestore (&priv->tx_lock, flags); -return NETDEV_TX_BUSY ; - }if (skb->dst && skb->dst->neighbour) {if (unlikely (!*to_ipoib_neigh (skb->dst->neighbour))) {
@@ -601,7 +585,6 @@ } out: -spin_unlock_irqrestore (&priv->tx_lock, flags);return NETDEV_TX_OK ; }
@@ -797,7 +780,7 @@ dev->addr_len =INFINIBAND_ALEN ; dev->type =ARPHRD_INFINIBAND ; dev->tx_queue_len =IPOIB_TX_RING_SIZE *2 ; - dev->features =NETIF_F_VLAN_CHALLENGED |NETIF_F_LLTX ; + dev->features =NETIF_F_VLAN_CHALLENGED ;/* MTU will be reset when mcast join happens */ dev->mtu =IPOIB_PACKET_SIZE -IPOIB_ENCAP_LEN ;
@@ -812,7 +795,6 @@ priv->dev = dev;spin_lock_init (&priv->lock); -spin_lock_init (&priv->tx_lock);init_MUTEX (&priv->mcast_mutex);init_MUTEX (&priv->vlan_mutex);
===== drivers/net/sungem.c 1.72 vs edited =====
drivers/net/sungem.c +54 -65
426 lines not on this page · raw message
===== drivers/net/sungem.h 1.18 vs edited =====
drivers/net/sungem.h +0 -1
@@ -953,7 +953,6 @@struct gem {spinlock_t lock; -spinlock_t tx_lock;void __iomem *regs;int rx_new, rx_old;int tx_new, tx_old;
===== drivers/net/tg3.c 1.227 vs edited =====
drivers/net/tg3.c +42 -71
359 lines not on this page · raw message
===== drivers/net/tg3.h 1.52 vs edited =====
drivers/net/tg3.h +3 -6
@@ -1980,12 +1980,11 @@* lock: Held during all operations except TX packet * processing. * -* tx_lock: Held during tg3_start_xmit{,_4gbug} and tg3_tx +* dev->xmit_lock: Held during tg3_start_xmit and tg3_tx * * If you want to shut up all asynchronous processing you must -* acquire both locks, 'lock' taken before 'tx_lock'. IRQs must -* be disabled to take 'lock' but only softirq disabling is -* necessary for acquisition of 'tx_lock'. +* acquire both locks, 'lock' taken before 'xmit_lock'. IRQs must +* be disabled to take either lock. */ spinlock_t lock;spinlock_t indirect_lock;
@@ -2003,8 +2002,6 @@u32 tx_prod;u32 tx_cons;u32 tx_pending; - -spinlock_t tx_lock;struct tg3_tx_buffer_desc *tx_ring;struct tx_ring_info *tx_buffers;
===== drivers/net/e1000/e1000.h 1.58 vs edited =====
drivers/net/e1000/e1000.h +0 -1
@@ -209,7 +209,6 @@/* TX */ struct e1000_desc_ring tx_ring; -spinlock_t tx_lock;uint32_t txd_cmd;uint32_t tx_int_delay;uint32_t tx_abs_int_delay;
===== drivers/net/e1000/e1000_main.c 1.145 vs edited =====
drivers/net/e1000/e1000_main.c +6 -19
@@ -520,9 +520,6 @@if (pci_using_dac) netdev->features |=NETIF_F_HIGHDMA ; -/* hard_start_xmit is safe against parallel locking */ - netdev->features |=NETIF_F_LLTX ; -/* before reading the EEPROM, reset the controller to * put the device in a known good starting state */
@@ -732,7 +729,6 @@atomic_set (&adapter->irq_sem,1 );spin_lock_init (&adapter->stats_lock); -spin_lock_init (&adapter->tx_lock);return 0 ; }
@@ -1308,7 +1304,7 @@/* Check for Promiscuous and All Multicast modes */ -spin_lock_irqsave (&adapter->tx_lock, flags); +spin_lock_irqsave (&netdev->xmit_lock, flags); rctl =E1000_READ_REG (hw,RCTL );
@@ -1359,7 +1355,7 @@if (hw->mac_type == e1000_82542_rev2_0)e1000_leave_82542_rst (adapter); -spin_unlock_irqrestore (&adapter->tx_lock, flags); +spin_unlock_irqrestore (&netdev->xmit_lock, flags); }/* Need to wait a few seconds after link up to get diagnostic information from
@@ -1786,6 +1782,8 @@ }#define TXD_USE_COUNT (S ,X ) (((S) >> (X)) + 1 ) + +/* Called with dev->xmit_lock held and interrupts disabled. */ static int e1000_xmit_frame (struct sk_buff *skb,struct net_device *netdev) {
@@ -1794,7 +1792,6 @@unsigned int max_txd_pwr =E1000_MAX_TXD_PWR ;unsigned int tx_flags =0 ;unsigned int len = skb->len; -unsigned long flags;unsigned int nr_frags =0 ;unsigned int mss =0 ;int count =0 ;
@@ -1838,18 +1835,10 @@if (adapter->pcix_82544) count += nr_frags; -local_irq_save (flags); -if (!spin_trylock (&adapter->tx_lock)) { -/* Collision - tell upper layer to requeue */ -local_irq_restore (flags); -return NETDEV_TX_LOCKED ; - } -/* need: count + 2 desc gap to keep tail from touching * head, otherwise try next time */ if (unlikely (E1000_DESC_UNUSED (&adapter->tx_ring) < count +2 )) {netif_stop_queue (netdev); -spin_unlock_irqrestore (&adapter->tx_lock, flags);return NETDEV_TX_BUSY ; }
@@ -1857,7 +1846,6 @@if (unlikely (e1000_82547_fifo_workaround (adapter, skb))) {netif_stop_queue (netdev);mod_timer (&adapter->tx_fifo_stall_timer, jiffies); -spin_unlock_irqrestore (&adapter->tx_lock, flags);return NETDEV_TX_BUSY ; } }
@@ -1884,7 +1872,6 @@if (unlikely (E1000_DESC_UNUSED (&adapter->tx_ring) <MAX_SKB_FRAGS +2 ))netif_stop_queue (netdev); -spin_unlock_irqrestore (&adapter->tx_lock, flags);return NETDEV_TX_OK ; }
@@ -2234,13 +2221,13 @@ tx_ring->next_to_clean = i; -spin_lock (&adapter->tx_lock); +spin_lock (&netdev->xmit_lock);if (unlikely (cleaned &&netif_queue_stopped (netdev) &&netif_carrier_ok (netdev)))netif_wake_queue (netdev); -spin_unlock (&adapter->tx_lock); +spin_unlock (&netdev->xmit_lock);return cleaned; }
===== include/linux/netdevice.h 1.96 vs edited =====
include/linux/netdevice.h +5 -4
@@ -76,7 +76,6 @@/* Driver transmit return codes */ #define NETDEV_TX_OK 0/* driver took care of packet */ #define NETDEV_TX_BUSY 1/* driver tx path was busy*/ -#define NETDEV_TX_LOCKED -1/* driver tx lock was already taken */ /* * Compute the worst case header length according to the protocols
@@ -415,7 +414,7 @@#define NETIF_F_HW_VLAN_FILTER 512/* Receive filtering on VLAN */ #define NETIF_F_VLAN_CHALLENGED 1024/* Device cannot handle VLAN packets */ #define NETIF_F_TSO 2048/* Can offload TCP/IP segmentation */ -#define NETIF_F_LLTX 4096/* LockLess TX */ +#define NETIF_F_LLTX 4096/* Do not grab xmit_lock during ->hard_start_xmit */ /* Called after device is detached from network. */ void (*uninit)(struct net_device *dev);
@@ -894,9 +893,11 @@static inline void netif_tx_disable (struct net_device *dev) { -spin_lock_bh (&dev->xmit_lock); +unsigned long flags; + +spin_lock_irqsave (&dev->xmit_lock, flags);netif_stop_queue (dev); -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irqrestore (&dev->xmit_lock, flags); }/* These functions live elsewhere (drivers/net/net_init.c, but related) */
===== net/atm/clip.c 1.43 vs edited =====
net/atm/clip.c +2 -2
@@ -97,7 +97,7 @@printk (KERN_CRIT "!clip_vcc->entry (clip_vcc %p)\n" ,clip_vcc);return ; } -spin_lock_bh (&entry->neigh->dev->xmit_lock);/* block clip_start_xmit() */ +spin_lock_irq (&entry->neigh->dev->xmit_lock);/* block clip_start_xmit() */ entry->neigh->used = jiffies;for (walk = &entry->vccs; *walk; walk = &(*walk)->next)if (*walk == clip_vcc) {
@@ -121,7 +121,7 @@printk (KERN_CRIT "ATMARP: unlink_clip_vcc failed (entry %p, vcc " "0x%p)\n" ,entry,clip_vcc); out: -spin_unlock_bh (&entry->neigh->dev->xmit_lock); +spin_unlock_irq (&entry->neigh->dev->xmit_lock); }/* The neighbour entry n->lock is held. */
===== net/core/dev.c 1.181 vs edited =====
net/core/dev.c +2 -2
@@ -1190,7 +1190,7 @@#define HARD_TX_LOCK (dev, cpu) { \ if ((dev->features & NETIF_F_LLTX) == 0) { \ - spin_lock(&dev->xmit_lock); \ + spin_lock_irq(&dev->xmit_lock); \ dev->xmit_lock_owner = cpu; \ } \ }
@@ -1198,7 +1198,7 @@#define HARD_TX_UNLOCK (dev) { \ if ((dev->features & NETIF_F_LLTX) == 0) { \ dev->xmit_lock_owner = -1; \ - spin_unlock(&dev->xmit_lock); \ + spin_unlock_irq(&dev->xmit_lock); \ } \ }
===== net/core/dev_mcast.c 1.4 vs edited =====
net/core/dev_mcast.c +13 -13
@@ -93,9 +93,9 @@void dev_mc_upload (struct net_device *dev) { -spin_lock_bh (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);__dev_mc_upload (dev); -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock); }/*
@@ -107,7 +107,7 @@int err =0 ;struct dev_mc_list *dmi, **dmip; -spin_lock_bh (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);for (dmip = &dev->mc_list; (dmi = *dmip) !=NULL ; dmip = &dmi->next) {/*
@@ -139,13 +139,13 @@*/ __dev_mc_upload (dev); -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock);return 0 ; } } err = -ENOENT ; done: -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock);return err; }
@@ -160,7 +160,7 @@ dmi1 = (struct dev_mc_list *)kmalloc (sizeof (*dmi),GFP_ATOMIC ); -spin_lock_bh (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);for (dmi = dev->mc_list; dmi !=NULL ; dmi = dmi->next) {if (memcmp (dmi->dmi_addr, addr, dmi->dmi_addrlen) ==0 && dmi->dmi_addrlen == alen) {
@@ -176,7 +176,7 @@ }if ((dmi = dmi1) ==NULL ) { -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock);return -ENOMEM ; }memcpy (dmi->dmi_addr, addr, alen);
@@ -189,11 +189,11 @@__dev_mc_upload (dev); -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock);return 0 ; done: -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock);if (dmi1)kfree (dmi1);return err;
@@ -205,7 +205,7 @@void dev_mc_discard (struct net_device *dev) { -spin_lock_bh (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);while (dev->mc_list !=NULL ) {struct dev_mc_list *tmp = dev->mc_list;
@@ -216,7 +216,7 @@ } dev->mc_count =0 ; -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock); }#ifdef CONFIG_PROC_FS
@@ -251,7 +251,7 @@struct dev_mc_list *m;struct net_device *dev = v; -spin_lock_bh (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);for (m = dev->mc_list; m; m = m->next) {int i;
@@ -263,7 +263,7 @@seq_putc (seq,'\n' ); } -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock);return 0 ; }
===== net/core/netpoll.c 1.24 vs edited =====
net/core/netpoll.c +3 -3
@@ -188,7 +188,7 @@return ; } -spin_lock (&np->dev->xmit_lock); +spin_lock_irq (&np->dev->xmit_lock); np->dev->xmit_lock_owner =smp_processor_id ();/*
@@ -197,7 +197,7 @@*/ if (netif_queue_stopped (np->dev)) { np->dev->xmit_lock_owner =-1 ; -spin_unlock (&np->dev->xmit_lock); +spin_unlock_irq (&np->dev->xmit_lock);netpoll_poll (np); goto repeat;
@@ -205,7 +205,7 @@ status = np->dev->hard_start_xmit (skb, np->dev); np->dev->xmit_lock_owner =-1 ; -spin_unlock (&np->dev->xmit_lock); +spin_unlock_irq (&np->dev->xmit_lock);/* transmit busy */ if (status) {
===== net/core/pktgen.c 1.21 vs edited =====
net/core/pktgen.c +2 -7
@@ -2664,12 +2664,11 @@ } } -spin_lock_bh (&odev->xmit_lock); +spin_lock_irq (&odev->xmit_lock);if (!netif_queue_stopped (odev)) {u64 now;atomic_inc (&(pkt_dev->skb->users)); -retry_now: ret = odev->hard_start_xmit (pkt_dev->skb, odev);if (likely (ret ==NETDEV_TX_OK )) { pkt_dev->last_ok =1 ;
@@ -2677,10 +2676,6 @@ pkt_dev->seq_num++; pkt_dev->tx_bytes += pkt_dev->cur_pkt_size; - }else if (ret ==NETDEV_TX_LOCKED - && (odev->features &NETIF_F_LLTX )) { -cpu_relax (); - goto retry_now; }else {/* Retry it next time */ atomic_dec (&(pkt_dev->skb->users));
@@ -2716,7 +2711,7 @@ pkt_dev->next_tx_ns =0 ; } -spin_unlock_bh (&odev->xmit_lock); +spin_unlock_irq (&odev->xmit_lock);/* If pkt_dev->count is zero, then run forever */ if ((pkt_dev->count !=0 ) && (pkt_dev->sofar >= pkt_dev->count)) {
===== net/sched/sch_generic.c 1.33 vs edited =====
net/sched/sch_generic.c +11 -20
@@ -99,16 +99,11 @@if ((skb = q->dequeue (q)) !=NULL ) {unsigned nolock = (dev->features &NETIF_F_LLTX );/* -* When the driver has LLTX set it does its own locking -* in start_xmit. No need to add additional overhead by -* locking again. These checks are worth it because -* even uncongested locks can be quite expensive. -* The driver can do trylock like here too, in case -* of lock congestion it should return -1 and the packet -* will be requeued. +* When the driver has LLTX set it does not require any +* locking in start_xmit. */ if (!nolock) { -if (!spin_trylock (&dev->xmit_lock)) { +if (!spin_trylock_irq (&dev->xmit_lock)) { collision:/* So, someone grabbed the driver. */
@@ -143,22 +138,18 @@if (ret ==NETDEV_TX_OK ) {if (!nolock) { dev->xmit_lock_owner =-1 ; -spin_unlock (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock); }spin_lock (&dev->queue_lock);return -1 ; } -if (ret ==NETDEV_TX_LOCKED && nolock) { -spin_lock (&dev->queue_lock); - goto collision; - } }/* NETDEV_TX_BUSY - we need to requeue */ /* Release the driver */ if (!nolock) { dev->xmit_lock_owner =-1 ; -spin_unlock (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock); }spin_lock (&dev->queue_lock); q = dev->qdisc;
@@ -186,7 +177,7 @@ {struct net_device *dev = (struct net_device *)arg; -spin_lock (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);if (dev->qdisc != &noop_qdisc) { if (netif_device_present(dev) && netif_running(dev) &&
@@ -200,7 +191,7 @@dev_hold (dev); } } -spin_unlock (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock);dev_put (dev); }
@@ -224,17 +215,17 @@static void dev_watchdog_up (struct net_device *dev) { -spin_lock_bh (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);__netdev_watchdog_up (dev); -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock); }static void dev_watchdog_down (struct net_device *dev) { -spin_lock_bh (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);if (del_timer (&dev->watchdog_timer))__dev_put (dev); -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock); }/* "NOOP" scheduler: the best scheduler, recommended for all interfaces
===== net/sched/sch_teql.c 1.16 vs edited =====
net/sched/sch_teql.c +3 -3
@@ -301,12 +301,12 @@switch (teql_resolve (skb, skb_res, slave)) {case 0 : -if (spin_trylock (&slave->xmit_lock)) { +if (spin_trylock_irq (&slave->xmit_lock)) { slave->xmit_lock_owner =smp_processor_id ();if (!netif_queue_stopped (slave) && slave->hard_start_xmit (skb, slave) ==0 ) { slave->xmit_lock_owner =-1 ; -spin_unlock (&slave->xmit_lock); +spin_unlock_irq (&slave->xmit_lock); master->slaves =NEXT_SLAVE (q);netif_wake_queue (dev); master->stats.tx_packets++;
@@ -314,7 +314,7 @@return 0 ; } slave->xmit_lock_owner =-1 ; -spin_unlock (&slave->xmit_lock); +spin_unlock_irq (&slave->xmit_lock); }if (netif_queue_stopped (dev)) busy =1 ;
Re: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 4 lines · David S. Miller
Ok, here is what something like this might look like.
In summary:
Quoted 9 lines
1)
dev->xmit_lockis now IRQ disabling instead of BH disabling
2) Drivers can usedev->xmit_lockin place of their privatedriver_priv->tx_lockand this is effectively the same gain LLTX gave drivers sans the race condition which started this thread.
3)NETDEV_TX_LOCKEDis gone
4) LLTX stays, but this means that the driver's TX routine is %100 lockless. It is only to be used by loopback and other queueless software devices such as tunnels.
Looks good to me and much cleaner than what I initially did. Thanks.
-Andi
David S. Miller
maintainerRe: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 2 lines · On Thu, 20 Jan 2005 04:14:23 +0100 Andi Kleen <[email protected]> wrote:
Looks good to me and much cleaner than what I initially did. Thanks.
Thanks for the review Andi.
Re: [PATCH]: was Re: LLTX and netif_stop_queue
In reply to David S. MillerLooks reasonable to me. I'll try the IPoIB changes out to make sure it still works.
Thanks,
Roland
David S. Miller
maintainerRe: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 2 lines · On Wed, 19 Jan 2005 19:43:42 -0800 Roland Dreier <[email protected]> wrote:
Looks reasonable to me. I'll try the IPoIB changes out to make sure it still works.
Great.
I actually tested my patch out, and there was a deadlock added to the tg3 and sungem drivers which is fixed in this updated version.
Basically, since the multicast code now takes dev->xmit_lock the ->set_multicast_list() routines should not grab it anymore.
To prevent ABBA deadlock, I had to remove the acquisition of the usual priv->lock in tg3 and sungem multicast list settings. Grabbing priv->lock was overkill and this new scheme of just holding dev->xmit_lock is OK, because:
1) Messing with the RX mode and multicast hash is safe even in parallel with RX packet processing.
2) All code paths that want to shut up all chip programming code paths still take both dev->xmit_lock and priv->lock thus still able to block out ->set_multicast_list() processing.
So I'm going to check this in and push upstream. Let me know if folks find any other errors.
Thanks.
===== Documentation/networking/netdevices.txt 1.5 vs edited =====
Documentation/networking/netdevices.txt +3 -6
@@ -45,10 +45,9 @@ Synchronization: dev->xmit_lock spinlock. When the driver sets NETIF_F_LLTX in dev->features this will be called without holding xmit_lock. In this case the driver - has to lock by itself when needed. It is recommended to use a try lock - for this and return -1 when the spin lock fails. - The locking there should also properly protect against - set_multicast_list + has to execute it's transmission routine in a completely lockless + manner. It is recommended only for queueless devices such + loopback and tunnels. Context: BHs disabled Notes: netif_queue_stopped() is guaranteed false Return codes:
@@ -56,8 +55,6 @@ o NETDEV_TX_BUSY Cannot transmit packet, try later Usually a bug, means queue start/stop flow control is broken in the driver. Note: the driver must NOT put the skb in its DMA ring. - o NETDEV_TX_LOCKED Locking failed, please retry quickly. - Only valid when NETIF_F_LLTX is set. dev->tx_timeout: Synchronization: dev->xmit_lock spinlock.
===== drivers/infiniband/ulp/ipoib/ipoib.h 1.3 vs edited =====
drivers/infiniband/ulp/ipoib/ipoib.h +4 -5
@@ -104,10 +104,10 @@ };/* -* Device private locking: tx_lock protects members used in TX fast -* path (and we use LLTX so upper layers don't do extra locking). -* lock protects everything else. lock nests inside of tx_lock (ie -* tx_lock must be acquired first if needed). +* Device private locking: netdev->xmit_lock protects members used +* in TX fast path. +* lock protects everything else. lock nests inside of xmit_lock (ie +* xmit_lock must be acquired first if needed). */ struct ipoib_dev_priv {spinlock_t lock;
@@ -150,7 +150,6 @@struct ipoib_buf *rx_ring; -spinlock_t tx_lock;struct ipoib_buf *tx_ring;unsigned tx_head;unsigned tx_tail;
===== drivers/infiniband/ulp/ipoib/ipoib_ib.c 1.4 vs edited =====
drivers/infiniband/ulp/ipoib/ipoib_ib.c +2 -2
@@ -247,12 +247,12 @@dev_kfree_skb_any (tx_req->skb); -spin_lock_irqsave (&priv->tx_lock, flags); +spin_lock_irqsave (&dev->xmit_lock, flags); ++priv->tx_tail;if (netif_queue_stopped (dev) && priv->tx_head - priv->tx_tail <=IPOIB_TX_RING_SIZE /2 )netif_wake_queue (dev); -spin_unlock_irqrestore (&priv->tx_lock, flags); +spin_unlock_irqrestore (&dev->xmit_lock, flags);if (wc->status !=IB_WC_SUCCESS && wc->status !=IB_WC_WR_FLUSH_ERR )
===== drivers/infiniband/ulp/ipoib/ipoib_main.c 1.3 vs edited =====
drivers/infiniband/ulp/ipoib/ipoib_main.c +4 -22
@@ -411,7 +411,7 @@/* * We can only be called from ipoib_start_xmit, so we're -* inside tx_lock -- no need to save/restore flags. +* inside dev->xmit_lock -- no need to save/restore flags. */ spin_lock (&priv->lock);
@@ -483,7 +483,7 @@/* * We can only be called from ipoib_start_xmit, so we're -* inside tx_lock -- no need to save/restore flags. +* inside dev->xmit_lock -- no need to save/restore flags. */ spin_lock (&priv->lock);
@@ -526,27 +526,11 @@spin_unlock (&priv->lock); } +/* Called with dev->xmit_lock held and IRQs disabled. */ static int ipoib_start_xmit (struct sk_buff *skb,struct net_device *dev) {struct ipoib_dev_priv *priv =netdev_priv (dev);struct ipoib_neigh *neigh; -unsigned long flags; - -local_irq_save (flags); -if (!spin_trylock (&priv->tx_lock)) { -local_irq_restore (flags); -return NETDEV_TX_LOCKED ; - } - -/* -* Check if our queue is stopped. Since we have the LLTX bit -* set, we can't rely on netif_stop_queue() preventing our -* xmit function from being called with a full queue. -*/ -if (unlikely (netif_queue_stopped (dev))) { -spin_unlock_irqrestore (&priv->tx_lock, flags); -return NETDEV_TX_BUSY ; - }if (skb->dst && skb->dst->neighbour) {if (unlikely (!*to_ipoib_neigh (skb->dst->neighbour))) {
@@ -601,7 +585,6 @@ } out: -spin_unlock_irqrestore (&priv->tx_lock, flags);return NETDEV_TX_OK ; }
@@ -797,7 +780,7 @@ dev->addr_len =INFINIBAND_ALEN ; dev->type =ARPHRD_INFINIBAND ; dev->tx_queue_len =IPOIB_TX_RING_SIZE *2 ; - dev->features =NETIF_F_VLAN_CHALLENGED |NETIF_F_LLTX ; + dev->features =NETIF_F_VLAN_CHALLENGED ;/* MTU will be reset when mcast join happens */ dev->mtu =IPOIB_PACKET_SIZE -IPOIB_ENCAP_LEN ;
@@ -812,7 +795,6 @@ priv->dev = dev;spin_lock_init (&priv->lock); -spin_lock_init (&priv->tx_lock);init_MUTEX (&priv->mcast_mutex);init_MUTEX (&priv->vlan_mutex);
===== drivers/net/sungem.c 1.72 vs edited =====
drivers/net/sungem.c +53 -69
434 lines not on this page · raw message
===== drivers/net/sungem.h 1.18 vs edited =====
drivers/net/sungem.h +0 -1
@@ -953,7 +953,6 @@struct gem {spinlock_t lock; -spinlock_t tx_lock;void __iomem *regs;int rx_new, rx_old;int tx_new, tx_old;
===== drivers/net/tg3.c 1.227 vs edited =====
drivers/net/tg3.c +41 -75
364 lines not on this page · raw message
===== drivers/net/tg3.h 1.52 vs edited =====
drivers/net/tg3.h +3 -6
@@ -1980,12 +1980,11 @@* lock: Held during all operations except TX packet * processing. * -* tx_lock: Held during tg3_start_xmit{,_4gbug} and tg3_tx +* dev->xmit_lock: Held during tg3_start_xmit and tg3_tx * * If you want to shut up all asynchronous processing you must -* acquire both locks, 'lock' taken before 'tx_lock'. IRQs must -* be disabled to take 'lock' but only softirq disabling is -* necessary for acquisition of 'tx_lock'. +* acquire both locks, 'lock' taken before 'xmit_lock'. IRQs must +* be disabled to take either lock. */ spinlock_t lock;spinlock_t indirect_lock;
@@ -2003,8 +2002,6 @@u32 tx_prod;u32 tx_cons;u32 tx_pending; - -spinlock_t tx_lock;struct tg3_tx_buffer_desc *tx_ring;struct tx_ring_info *tx_buffers;
===== drivers/net/e1000/e1000.h 1.58 vs edited =====
drivers/net/e1000/e1000.h +0 -1
@@ -209,7 +209,6 @@/* TX */ struct e1000_desc_ring tx_ring; -spinlock_t tx_lock;uint32_t txd_cmd;uint32_t tx_int_delay;uint32_t tx_abs_int_delay;
===== drivers/net/e1000/e1000_main.c 1.145 vs edited =====
drivers/net/e1000/e1000_main.c +11 -22
@@ -291,7 +291,9 @@e1000_phy_reset (&adapter->hw); } +spin_lock_irq (&netdev->xmit_lock);e1000_set_multi (netdev); +spin_unlock_irq (&netdev->xmit_lock);e1000_restore_vlan (adapter);
@@ -520,9 +522,6 @@if (pci_using_dac) netdev->features |=NETIF_F_HIGHDMA ; -/* hard_start_xmit is safe against parallel locking */ - netdev->features |=NETIF_F_LLTX ; -/* before reading the EEPROM, reset the controller to * put the device in a known good starting state */
@@ -732,7 +731,6 @@atomic_set (&adapter->irq_sem,1 );spin_lock_init (&adapter->stats_lock); -spin_lock_init (&adapter->tx_lock);return 0 ; }
@@ -1293,6 +1291,8 @@* list or the network interface flags are updated. This routine is * responsible for configuring the hardware for proper multicast, * promiscuous mode, and all-multi behavior. +* +* Called with netdev->xmit_lock held and IRQs disabled. **/ static void
@@ -1304,12 +1304,9 @@uint32_t rctl;uint32_t hash_value;int i; -unsigned long flags;/* Check for Promiscuous and All Multicast modes */ -spin_lock_irqsave (&adapter->tx_lock, flags); - rctl =E1000_READ_REG (hw,RCTL );if (netdev->flags &IFF_PROMISC ) {
@@ -1358,8 +1355,6 @@if (hw->mac_type == e1000_82542_rev2_0)e1000_leave_82542_rst (adapter); - -spin_unlock_irqrestore (&adapter->tx_lock, flags); }/* Need to wait a few seconds after link up to get diagnostic information from
@@ -1786,6 +1781,8 @@ }#define TXD_USE_COUNT (S ,X ) (((S) >> (X)) + 1 ) + +/* Called with dev->xmit_lock held and interrupts disabled. */ static int e1000_xmit_frame (struct sk_buff *skb,struct net_device *netdev) {
@@ -1794,7 +1791,6 @@unsigned int max_txd_pwr =E1000_MAX_TXD_PWR ;unsigned int tx_flags =0 ;unsigned int len = skb->len; -unsigned long flags;unsigned int nr_frags =0 ;unsigned int mss =0 ;int count =0 ;
@@ -1838,18 +1834,10 @@if (adapter->pcix_82544) count += nr_frags; -local_irq_save (flags); -if (!spin_trylock (&adapter->tx_lock)) { -/* Collision - tell upper layer to requeue */ -local_irq_restore (flags); -return NETDEV_TX_LOCKED ; - } -/* need: count + 2 desc gap to keep tail from touching * head, otherwise try next time */ if (unlikely (E1000_DESC_UNUSED (&adapter->tx_ring) < count +2 )) {netif_stop_queue (netdev); -spin_unlock_irqrestore (&adapter->tx_lock, flags);return NETDEV_TX_BUSY ; }
@@ -1857,7 +1845,6 @@if (unlikely (e1000_82547_fifo_workaround (adapter, skb))) {netif_stop_queue (netdev);mod_timer (&adapter->tx_fifo_stall_timer, jiffies); -spin_unlock_irqrestore (&adapter->tx_lock, flags);return NETDEV_TX_BUSY ; } }
@@ -1884,7 +1871,6 @@if (unlikely (E1000_DESC_UNUSED (&adapter->tx_ring) <MAX_SKB_FRAGS +2 ))netif_stop_queue (netdev); -spin_unlock_irqrestore (&adapter->tx_lock, flags);return NETDEV_TX_OK ; }
@@ -2234,13 +2220,13 @@ tx_ring->next_to_clean = i; -spin_lock (&adapter->tx_lock); +spin_lock (&netdev->xmit_lock);if (unlikely (cleaned &&netif_queue_stopped (netdev) &&netif_carrier_ok (netdev)))netif_wake_queue (netdev); -spin_unlock (&adapter->tx_lock); +spin_unlock (&netdev->xmit_lock);return cleaned; }
@@ -2819,7 +2805,10 @@if (wufc) {e1000_setup_rctl (adapter); + +spin_lock_irq (&netdev->xmit_lock);e1000_set_multi (netdev); +spin_unlock_irq (&netdev->xmit_lock);/* turn on all-multi mode if wake on multicast is enabled */ if (adapter->wol &E1000_WUFC_MC ) {
===== include/linux/netdevice.h 1.96 vs edited =====
include/linux/netdevice.h +5 -4
@@ -76,7 +76,6 @@/* Driver transmit return codes */ #define NETDEV_TX_OK 0/* driver took care of packet */ #define NETDEV_TX_BUSY 1/* driver tx path was busy*/ -#define NETDEV_TX_LOCKED -1/* driver tx lock was already taken */ /* * Compute the worst case header length according to the protocols
@@ -415,7 +414,7 @@#define NETIF_F_HW_VLAN_FILTER 512/* Receive filtering on VLAN */ #define NETIF_F_VLAN_CHALLENGED 1024/* Device cannot handle VLAN packets */ #define NETIF_F_TSO 2048/* Can offload TCP/IP segmentation */ -#define NETIF_F_LLTX 4096/* LockLess TX */ +#define NETIF_F_LLTX 4096/* Do not grab xmit_lock during ->hard_start_xmit */ /* Called after device is detached from network. */ void (*uninit)(struct net_device *dev);
@@ -894,9 +893,11 @@static inline void netif_tx_disable (struct net_device *dev) { -spin_lock_bh (&dev->xmit_lock); +unsigned long flags; + +spin_lock_irqsave (&dev->xmit_lock, flags);netif_stop_queue (dev); -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irqrestore (&dev->xmit_lock, flags); }/* These functions live elsewhere (drivers/net/net_init.c, but related) */
===== net/atm/clip.c 1.43 vs edited =====
net/atm/clip.c +2 -2
@@ -97,7 +97,7 @@printk (KERN_CRIT "!clip_vcc->entry (clip_vcc %p)\n" ,clip_vcc);return ; } -spin_lock_bh (&entry->neigh->dev->xmit_lock);/* block clip_start_xmit() */ +spin_lock_irq (&entry->neigh->dev->xmit_lock);/* block clip_start_xmit() */ entry->neigh->used = jiffies;for (walk = &entry->vccs; *walk; walk = &(*walk)->next)if (*walk == clip_vcc) {
@@ -121,7 +121,7 @@printk (KERN_CRIT "ATMARP: unlink_clip_vcc failed (entry %p, vcc " "0x%p)\n" ,entry,clip_vcc); out: -spin_unlock_bh (&entry->neigh->dev->xmit_lock); +spin_unlock_irq (&entry->neigh->dev->xmit_lock); }/* The neighbour entry n->lock is held. */
===== net/core/dev.c 1.181 vs edited =====
net/core/dev.c +2 -2
@@ -1190,7 +1190,7 @@#define HARD_TX_LOCK (dev, cpu) { \ if ((dev->features & NETIF_F_LLTX) == 0) { \ - spin_lock(&dev->xmit_lock); \ + spin_lock_irq(&dev->xmit_lock); \ dev->xmit_lock_owner = cpu; \ } \ }
@@ -1198,7 +1198,7 @@#define HARD_TX_UNLOCK (dev) { \ if ((dev->features & NETIF_F_LLTX) == 0) { \ dev->xmit_lock_owner = -1; \ - spin_unlock(&dev->xmit_lock); \ + spin_unlock_irq(&dev->xmit_lock); \ } \ }
===== net/core/dev_mcast.c 1.4 vs edited =====
net/core/dev_mcast.c +13 -13
@@ -93,9 +93,9 @@void dev_mc_upload (struct net_device *dev) { -spin_lock_bh (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);__dev_mc_upload (dev); -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock); }/*
@@ -107,7 +107,7 @@int err =0 ;struct dev_mc_list *dmi, **dmip; -spin_lock_bh (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);for (dmip = &dev->mc_list; (dmi = *dmip) !=NULL ; dmip = &dmi->next) {/*
@@ -139,13 +139,13 @@*/ __dev_mc_upload (dev); -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock);return 0 ; } } err = -ENOENT ; done: -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock);return err; }
@@ -160,7 +160,7 @@ dmi1 = (struct dev_mc_list *)kmalloc (sizeof (*dmi),GFP_ATOMIC ); -spin_lock_bh (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);for (dmi = dev->mc_list; dmi !=NULL ; dmi = dmi->next) {if (memcmp (dmi->dmi_addr, addr, dmi->dmi_addrlen) ==0 && dmi->dmi_addrlen == alen) {
@@ -176,7 +176,7 @@ }if ((dmi = dmi1) ==NULL ) { -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock);return -ENOMEM ; }memcpy (dmi->dmi_addr, addr, alen);
@@ -189,11 +189,11 @@__dev_mc_upload (dev); -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock);return 0 ; done: -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock);if (dmi1)kfree (dmi1);return err;
@@ -205,7 +205,7 @@void dev_mc_discard (struct net_device *dev) { -spin_lock_bh (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);while (dev->mc_list !=NULL ) {struct dev_mc_list *tmp = dev->mc_list;
@@ -216,7 +216,7 @@ } dev->mc_count =0 ; -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock); }#ifdef CONFIG_PROC_FS
@@ -251,7 +251,7 @@struct dev_mc_list *m;struct net_device *dev = v; -spin_lock_bh (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);for (m = dev->mc_list; m; m = m->next) {int i;
@@ -263,7 +263,7 @@seq_putc (seq,'\n' ); } -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock);return 0 ; }
===== net/core/netpoll.c 1.24 vs edited =====
net/core/netpoll.c +3 -3
@@ -188,7 +188,7 @@return ; } -spin_lock (&np->dev->xmit_lock); +spin_lock_irq (&np->dev->xmit_lock); np->dev->xmit_lock_owner =smp_processor_id ();/*
@@ -197,7 +197,7 @@*/ if (netif_queue_stopped (np->dev)) { np->dev->xmit_lock_owner =-1 ; -spin_unlock (&np->dev->xmit_lock); +spin_unlock_irq (&np->dev->xmit_lock);netpoll_poll (np); goto repeat;
@@ -205,7 +205,7 @@ status = np->dev->hard_start_xmit (skb, np->dev); np->dev->xmit_lock_owner =-1 ; -spin_unlock (&np->dev->xmit_lock); +spin_unlock_irq (&np->dev->xmit_lock);/* transmit busy */ if (status) {
===== net/core/pktgen.c 1.21 vs edited =====
net/core/pktgen.c +2 -7
@@ -2664,12 +2664,11 @@ } } -spin_lock_bh (&odev->xmit_lock); +spin_lock_irq (&odev->xmit_lock);if (!netif_queue_stopped (odev)) {u64 now;atomic_inc (&(pkt_dev->skb->users)); -retry_now: ret = odev->hard_start_xmit (pkt_dev->skb, odev);if (likely (ret ==NETDEV_TX_OK )) { pkt_dev->last_ok =1 ;
@@ -2677,10 +2676,6 @@ pkt_dev->seq_num++; pkt_dev->tx_bytes += pkt_dev->cur_pkt_size; - }else if (ret ==NETDEV_TX_LOCKED - && (odev->features &NETIF_F_LLTX )) { -cpu_relax (); - goto retry_now; }else {/* Retry it next time */ atomic_dec (&(pkt_dev->skb->users));
@@ -2716,7 +2711,7 @@ pkt_dev->next_tx_ns =0 ; } -spin_unlock_bh (&odev->xmit_lock); +spin_unlock_irq (&odev->xmit_lock);/* If pkt_dev->count is zero, then run forever */ if ((pkt_dev->count !=0 ) && (pkt_dev->sofar >= pkt_dev->count)) {
===== net/sched/sch_generic.c 1.33 vs edited =====
net/sched/sch_generic.c +11 -21
@@ -99,17 +99,11 @@if ((skb = q->dequeue (q)) !=NULL ) {unsigned nolock = (dev->features &NETIF_F_LLTX );/* -* When the driver has LLTX set it does its own locking -* in start_xmit. No need to add additional overhead by -* locking again. These checks are worth it because -* even uncongested locks can be quite expensive. -* The driver can do trylock like here too, in case -* of lock congestion it should return -1 and the packet -* will be requeued. +* When the driver has LLTX set it does not require any +* locking in start_xmit. */ if (!nolock) { -if (!spin_trylock (&dev->xmit_lock)) { - collision: +if (!spin_trylock_irq (&dev->xmit_lock)) {/* So, someone grabbed the driver. */ /* It may be transient configuration error,
@@ -143,22 +137,18 @@if (ret ==NETDEV_TX_OK ) {if (!nolock) { dev->xmit_lock_owner =-1 ; -spin_unlock (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock); }spin_lock (&dev->queue_lock);return -1 ; } -if (ret ==NETDEV_TX_LOCKED && nolock) { -spin_lock (&dev->queue_lock); - goto collision; - } }/* NETDEV_TX_BUSY - we need to requeue */ /* Release the driver */ if (!nolock) { dev->xmit_lock_owner =-1 ; -spin_unlock (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock); }spin_lock (&dev->queue_lock); q = dev->qdisc;
@@ -186,7 +176,7 @@ {struct net_device *dev = (struct net_device *)arg; -spin_lock (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);if (dev->qdisc != &noop_qdisc) { if (netif_device_present(dev) && netif_running(dev) &&
@@ -200,7 +190,7 @@dev_hold (dev); } } -spin_unlock (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock);dev_put (dev); }
@@ -224,17 +214,17 @@static void dev_watchdog_up (struct net_device *dev) { -spin_lock_bh (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);__netdev_watchdog_up (dev); -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock); }static void dev_watchdog_down (struct net_device *dev) { -spin_lock_bh (&dev->xmit_lock); +spin_lock_irq (&dev->xmit_lock);if (del_timer (&dev->watchdog_timer))__dev_put (dev); -spin_unlock_bh (&dev->xmit_lock); +spin_unlock_irq (&dev->xmit_lock); }/* "NOOP" scheduler: the best scheduler, recommended for all interfaces
===== net/sched/sch_teql.c 1.16 vs edited =====
net/sched/sch_teql.c +3 -3
@@ -301,12 +301,12 @@switch (teql_resolve (skb, skb_res, slave)) {case 0 : -if (spin_trylock (&slave->xmit_lock)) { +if (spin_trylock_irq (&slave->xmit_lock)) { slave->xmit_lock_owner =smp_processor_id ();if (!netif_queue_stopped (slave) && slave->hard_start_xmit (skb, slave) ==0 ) { slave->xmit_lock_owner =-1 ; -spin_unlock (&slave->xmit_lock); +spin_unlock_irq (&slave->xmit_lock); master->slaves =NEXT_SLAVE (q);netif_wake_queue (dev); master->stats.tx_packets++;
@@ -314,7 +314,7 @@return 0 ; } slave->xmit_lock_owner =-1 ; -spin_unlock (&slave->xmit_lock); +spin_unlock_irq (&slave->xmit_lock); }if (netif_queue_stopped (dev)) busy =1 ;
Re: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 2 lines · David S. Miller
So I'm going to check this in and push upstream. Let me know if folks find any other errors.
Hi Dave
Sorry to be the one with bad news.
I am in favour of this idea, but there are still issues that need to be sorted out.
Quoted 9 lines
=====
drivers/net/sungem.c1.72 vs edited =====drivers/net/sungem.c +0
-1@@ -932,12 +932,12 @@ readl(gp->regs + MAC_RXCFG));spin_lock_irq (&gp->lock); -spin_lock (&gp->tx_lock);
drivers/net/sungem.c line 943
Quoted 10 lines
drivers/net/sungem.c +2
-1+spin_lock (&dev->xmit_lock); gp->reset_task_pending =2 ;schedule_work (&gp->reset_task); -spin_unlock (&gp->tx_lock); +spin_unlock (&dev->xmit_lock);spin_unlock_irq (&gp->lock); }
->tx_timeout() can't take dev->xmit_lock, since dev_watchdog already has it held.
A lot more serious is the fact that ->tx_timeout() and ->hard_start_xmit() are no longer allowed to do this:
spin_lock_irq()
...
spin_unlock_irq()
since that would leave us with irq's enabled while still holding the xmit_lock.
This would have to be fixed for non-LLTX drivers as well.
-Tommy
David S. Miller
maintainerRe: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 10 lines · On Thu, 20 Jan 2005 14:51:25 +0100 Tommy Christensen…
A lot more serious is the fact that ->tx_timeout() and ->hard_start_xmit() are no longer allowed to do this:
spin_lock_irq()
...
spin_unlock_irq()since that would leave us with irq's enabled while still holding the
xmit_lock.
This would have to be fixed for non-LLTX drivers as well.
Even worse is that this breaks the acenic driver too because it does this:
unsigned long maxjiff = jiffies + 3*HZ;
if (time_before(jiffies, maxjiff)) {
barrier();
cpu_relax();
goto restart;
}in it's ->hard_start_xmit() routine.
I was auditing spin_lock_irq() usage in ->hard_start_xmit() routines when I caught this.
This one isn't impossible to fix though. We can replace the jiffies games with a udelay/mdelay and a "maxloop" variable. Any takers?
I'll keep working on the spin_lock_irq() audit then work on the other problems Tommy found.
Re: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 11 lines · David S. Miller
Even worse is that this breaks the acenic driver too because it does this:
unsigned long maxjiff = jiffies + 3*HZ; if (time_before(jiffies, maxjiff)) { barrier(); cpu_relax(); goto restart; }
Quoted 8 lines
in it's ->hard_start_xmit() routine.
I was auditing
spin_lock_irq()usage in ->hard_start_xmit() routines when I caught this.This one isn't impossible to fix though. We can replace the jiffies games with a udelay/mdelay and a "maxloop" variable. Any takers?
I saw the next mail suggesting to revert the changes because of more fallout.
But if/when acenic needs changes like those proposed above, I can implement and test proposed changes if folks have the patience to wait a week or so. acenic is no where on my "official HP supported NICs" list but I happen to have the HW/infrastructure to test them.
grant
David S. Miller
maintainerRe: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 1 line · On Thu, 20 Jan 2005 13:56:50 -0800 Grant Grundler <[email protected]> wrote:
I saw the next mail suggesting to revert the changes because of more fallout.
Yeah, that's what I ended up just pushing to Linus.
Re: [PATCH]: was Re: LLTX and netif_stop_queue
So what's the plan here with LLTX? As far as I can tell, 2.6.11-rc2 went out with the changes that disable IRQs in hard_start_xmit...
- R.
David S. Miller
maintainerRe: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 2 lines · On Fri, 21 Jan 2005 19:17:07 -0800 Roland Dreier <[email protected]> wrote:
So what's the plan here with LLTX? As far as I can tell, 2.6.11-rc2 went out with the changes that disable IRQs in
hard_start_xmit...
Linus just didn't pull the backout of that stuff in time. I'll repush it to him tonight.
David S. Miller
maintainerRe: [PATCH]: was Re: LLTX and netif_stop_queue
In reply to Tommy ChristensenQuoted 2 lines · On Thu, 20 Jan 2005 14:51:25 +0100 Tommy Christensen…
A lot more serious is the fact that ->tx_timeout() and ->hard_start_xmit() are no longer allowed to do this:
Sigh, there are even more issues. Now dev_kfree_skb() is illegal in ->hard_start_xmit() because interrupts are disabled, and there are many such cases in the simpler net drivers.
I think we might need to revert the change over to IRQ disabling dev->xmit_lock and rethink this. This is just too much fallout.
Re: [PATCH]: was Re: LLTX and netif_stop_queue
In reply to David S. MillerYou might want to enforce that LLTX devices like tunnels need to have no transmit queue (tx_queue_len == 0)?
I can run a test on 8 way with e1000 if it would help.
Re: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 2 lines · Stephen Hemminger
You might want to enforce that LLTX devices like tunnels need to have no transmit queue (
tx_queue_len== 0)?
I didn't look at the original LLTX patch, so I'm speaking out of ignorance, but since we're on this subject anyway: I noticed that the ip_gre tunneling driver contains this comment:
<quote>
1. The most important issue is detecting local dead loops. They would cause complete host lockup in transmit, which would be "resolved" by stack overflow or, if queueing is enabled, with infinite looping in net_bh.
We cannot track such dead loops during route installation, it is infeasible task. The most general solutions would be to keep skb->encapsulation counter (sort of local ttl), and silently drop packet when it expires. It is the best solution, but it supposes maintaing new variable in ALL skb, even if no tunneling is used.
Current solution: t->recursion lock breaks dead loops. It looks like dev->tbusy flag, but I preferred new variable, because the semantics is different. One day, when hard_start_xmit will be multithreaded we will have to use skb->encapsulation.
</quote>
If multiple CPUs can call into the tunneling drivers without taking any locks, we'd need some extra locking in there, or just do what Alexey describes and keep track of recursion in the skb.
The recursion detection in dev_queue_xmit would perhaps also want to use such a per-skb mechanism instead of using dev->xmit_lock_owner.
(You really _do_ want the protection that this mechanism offers.)
--L
David S. Miller
maintainerRe: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 3 lines · On Fri, 21 Jan 2005 11:54:52 +0100 Lennert Buytenhek <[email protected]>…
If multiple CPUs can call into the tunneling drivers without taking any locks, we'd need some extra locking in there, or just do what Alexey describes and keep track of recursion in the skb.
Another idea is that, just like how loopback made it's statistics per-cpu for LLTX support, this recursion variable could be per-cpu as well.
Re: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 7 lines · David S. Miller
Quoted 3 lines
If multiple CPUs can call into the tunneling drivers without taking any locks, we'd need some extra locking in there, or just do what Alexey describes and keep track of recursion in the skb.
Another idea is that, just like how loopback made it's statistics per-cpu for LLTX support, this recursion variable could be per-cpu as well.
I've thought about this a bit, and the only sane way of doing recursion detection that doesn't involve 'struct net_device' would be to keep track of the recursion depth (perhaps per-CPU as you suggest) and tossing the packet when it exceeds some random value, right?
To reproduce the current behaviour more closely you'd have to keep a small per-CPU array of 'struct net_device *' pointers as a kind of recursion stack, and toss the packet when you hit a net_device that's already on the list. But that seems like slight overkill.
--L
David S. Miller
maintainerRe: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 4 lines · On Wed, 26 Jan 2005 14:25:12 +0100 Lennert Buytenhek <[email protected]>…
I've thought about this a bit, and the only sane way of doing recursion detection that doesn't involve 'struct
net_device' would be to keep track of the recursion depth (perhaps per-CPU as you suggest) and tossing the packet when it exceeds some random value, right?
Yes, that's the idea.
Quoted 4 lines
To reproduce the current behaviour more closely you'd have to keep a small per-CPU array of 'struct
net_device*' pointers as a kind of recursion stack, and toss the packet when you hit anet_devicethat's already on the list. But that seems like slight overkill.
Indeed.
Re: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 9 lines · David S. Miller
Quoted 4 lines · On Wed, 26 Jan 2005 14:25:12 +0100 Lennert Buytenhek <[email protected]>…
I've thought about this a bit, and the only sane way of doing recursion detection that doesn't involve 'struct
net_device' would be to keep track of the recursion depth (perhaps per-CPU as you suggest) and tossing the packet when it exceeds some random value, right?Yes, that's the idea.
per CPU only works in preemptive kernel if you have preemption disabled all the time. Do you?
Seems not likely to me.
-Andi
David S. Miller
maintainerRe: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 6 lines · On Thu, 27 Jan 2005 08:16:45 +0100 Andi Kleen <[email protected]> wrote:
Quoted 1 line
Yes, that's the idea.
per CPU only works in preemptive kernel if you have preemption disabled all the time. Do you?
Seems not likely to me.
BH is disabled in these code paths (specifically we're talking about ->hard_start_xmit()), as that is where the recursion check goes.
Otherwise, loopback's LLTX is broken as it relies on this property as well.
Re: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 5 lines · David S. Miller
Quoted 2 lines · On Thu, 27 Jan 2005 08:16:45 +0100 Andi Kleen <[email protected]> wrote:
Quoted 1 line
Yes, that's the idea.
Quoted 8 lines
Quoted 4 lines
per CPU only works in preemptive kernel if you have preemption disabled all the time. Do you?
Seems not likely to me.
BH is disabled in these code paths (specifically we're talking about ->hard_start_xmit()), as that is where the recursion check goes.
Hmm, but hard_start_xmit is allowed to enable preemption again, isn't it?
An safer alternative may be to change the cpu mask of the current process temporarily.
-Andi
Re: [PATCH]: was Re: LLTX and netif_stop_queue
In reply to David S. MillerReminder: double-check that the locking rules in Documentation/networking/netdevices.txt still apply; update the text file where you changed things.
Re: [PATCH]: was Re: LLTX and netif_stop_queue
In reply to David S. MillerOne thing i didnt quiet follow Dave in some of the drivers, example in the e1000:
-----
/* Called with dev->xmit_lock held and interrupts disabled. */
static int
e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
---
Who calls that with dev->xmit_lock held and interrupts disabled? Shouldnt the spin_unlock(&netdev->xmit_lock); be right at the top of that routine now?
cheers,
jamal
David S. Miller
maintainerRe: [PATCH]: was Re: LLTX and netif_stop_queue
Quoted 7 lines · On 19 Jan 2005 23:01:47 -0500 jamal <[email protected]> wrote:
-----
/* Called withdev->xmit_lockheld and interrupts disabled. */
static int
e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
---Who calls that with
dev->xmit_lockheld and interrupts disabled?
qdisc_restart() and dev_queue_xmit(), via netdev->hard_start_xmit().
Quoted 2 lines
Shouldnt the
spin_unlock(&netdev->xmit_lock); be right at the top of that routine now?
Nope, the idea now is that netdev->xmit_lock replaces the driver private tx_lock