explict hold dev tx lock
Subsystems (3)
RFC 1/1 explict hold dev tx lock
While trying to port my batching changes to net-2.6.24 from this morning i realized this is something i had wanted to probe people on....
Challenge:
For N Cpus, with full throttle traffic on all N CPUs, funneling traffic to the same ethernet device, the devices queue lock is contended by all N CPUs constantly. The TX lock is only contended by a max of 2 CPUS. In the current mode of operation, after all the work of entering the dequeue region, we may endup aborting the path if we are unable to get the tx lock and go back to contend for the queue lock. As N goes up, this gets worse.
Testing:
I did some testing with a 4 cpu (2xdual core) with no irq binding. I run about 10 runs of 30M packets each from the stack with a udp app i wrote which is intended to run keep all 4 cpus busy - and to my suprise i found that we only bail out less than 0.1%. I may need a better test case.
Changes:
I made changes to the code path as defined in the patch included to and noticed a slight increase (2-3%) in performance with both e1000 and tg3; which was a relief because i thought the spinlock_irq (which is needed because some drivers grab tx lock in interupts) may have negative effects. The fact it didnt reduce performance was a good thing.
Note: This is the highest end machine ive ever laid hands on, so this may be misleading.
So - what side effects do people see in doing this? If none, i will clean it up and submit.
cheers,
jamal
include/linux/netdevice.h +12 -0
@@ -1271,6 +1271,12 @@ static inline void netif_tx_lock(struct net_device *dev) dev->xmit_lock_owner =smp_processor_id (); } +static inline void netif_tx_lock_irq (struct net_device *dev) +{ +spin_lock_irq (&dev->_xmit_lock); + dev->xmit_lock_owner =smp_processor_id (); +} +static inline void netif_tx_lock_bh (struct net_device *dev) {spin_lock_bh (&dev->_xmit_lock);
@@ -1291,6 +1297,12 @@ static inline void netif_tx_unlock(struct net_device *dev)spin_unlock (&dev->_xmit_lock); } +static inline void netif_tx_unlock_irq (struct net_device *dev) +{ + dev->xmit_lock_owner =-1 ; +spin_unlock_irq (&dev->_xmit_lock); +} +static inline void netif_tx_unlock_bh (struct net_device *dev) { dev->xmit_lock_owner =-1 ;
net/sched/sch_generic.c +5 -16
@@ -134,34 +134,23 @@ static inline int qdisc_restart(struct net_device *dev) {struct Qdisc *q = dev->qdisc;struct sk_buff *skb; -unsigned lockless; +unsigned lockless = (dev->features &NETIF_F_LLTX );int ret;/* Dequeue packet */ if (unlikely ((skb =dev_dequeue_skb (dev, q)) ==NULL ))return 0 ; -/* -* When the driver has LLTX set, it does its own locking in -* start_xmit. These checks are worth it because even uncongested -* locks can be quite expensive. The driver can do a trylock, as -* is being done here; in case of lock contention it should return -* NETDEV_TX_LOCKED and the packet will be requeued. -*/ - lockless = (dev->features &NETIF_F_LLTX ); - -if (!lockless && !netif_tx_trylock (dev)) { -/* Another CPU grabbed the driver tx lock */ -return handle_dev_cpu_collision (skb, dev, q); - } -/* And release queue */ spin_unlock (&dev->queue_lock); +if (!lockless) +netif_tx_lock_irq (dev); + ret =dev_hard_start_xmit (skb, dev);if (!lockless) -netif_tx_unlock (dev); +netif_tx_unlock_irq (dev);spin_lock (&dev->queue_lock); q = dev->qdisc;
David Miller
maintainerFrom: jamal <[email protected]>
Date: Sun, 16 Sep 2007 12:14:34 -0400
Quoted 11 lines
Changes:
I made changes to the code path as defined in the patch included to and noticed a slight increase (2-3%) in performance with both e1000 and tg3; which was a relief because i thought thespinlock_irq(which is needed because some drivers grab tx lock in interupts) may have negative effects. The fact it didnt reduce performance was a good thing.
Note: This is the highest end machine ive ever laid hands on, so this may be misleading.So - what side effects do people see in doing this? If none, i will clean it up and submit.
I tried this 4 years ago, it doesn't work. :-)
Many drivers, particularly very old ones that PIO packets into a device which can take a long time, absolutely depend upon interrupts being enabled fully during ->hard_start_xmit() so that other high periority devices (such as simpler serial controllers) can have their interrupts serviced during this slow operation.
I don't think we want to do it anyways, whatever performance we gain from it is offset by the badness of disabling interrupts during this reasonably length stretch of code.
The -rt folks as a result would notice this too and spank us :-)
Quoted 8 lines · David Miller
From: jamal <[email protected]>
Date: Sun, 16 Sep 2007 12:14:34 -0400Quoted 2 lines
So - what side effects do people see in doing this? If none, i will clean it up and submit.
I tried this 4 years ago, it doesn't work. :-)
;->
[good reasons removed here]
Quoted 5 lines
I don't think we want to do it anyways, whatever performance we gain from it is offset by the badness of disabling interrupts during this reasonably length stretch of code.
The -rt folks as a result would notice this too and spank us :-)
indeed.
Ok, maybe i am thinking too hard with that patch, so help me out:-> When i looked at that code path as it is today: i felt the softirq could be interupted on the same CPU it is running on while it already grabbed that tx lock (if the trylock succeeds) and that the hardirq code when attempting to grab the lock would result in a deadlock.
Did i misread that?
When i experimented with tg3 and e1000 i did not see any such problems with the non irq version of the lock.
cheers,
jamal
Quoted 2 lines · jamal
indeed.
Ok, maybe i am thinking too hard with that patch, so help me out:->
Ok, that was probably too much of an explanation. What i should say is if i grabbed the lock explicitly without disabling irqs it wont be much different than what is done today and should always work. No?
cheers,
jamal
Quoted 4 lines · jamal
What i should say is
if i grabbed the lock explicitly without disabling irqs it wont be much different than what is done today and should always work. No?
And to be more explicit, heres a patch using the macros from previous patch. So far tested on 3 NICs.
cheers,
jamal
net/sched/sch_generic.c +2 -18
@@ -134,34 +134,18 @@ static inline int qdisc_restart(struct net_device *dev) {struct Qdisc *q = dev->qdisc;struct sk_buff *skb; -unsigned lockless;int ret;/* Dequeue packet */ if (unlikely ((skb =dev_dequeue_skb (dev, q)) ==NULL ))return 0 ; -/* -* When the driver has LLTX set, it does its own locking in -* start_xmit. These checks are worth it because even uncongested -* locks can be quite expensive. The driver can do a trylock, as -* is being done here; in case of lock contention it should return -* NETDEV_TX_LOCKED and the packet will be requeued. -*/ - lockless = (dev->features &NETIF_F_LLTX ); - -if (!lockless && !netif_tx_trylock (dev)) { -/* Another CPU grabbed the driver tx lock */ -return handle_dev_cpu_collision (skb, dev, q); - } -/* And release queue */ spin_unlock (&dev->queue_lock); +HARD_TX_LOCK (dev,smp_processor_id ()); ret =dev_hard_start_xmit (skb, dev); - -if (!lockless) -netif_tx_unlock (dev); +HARD_TX_UNLOCK (dev);spin_lock (&dev->queue_lock); q = dev->qdisc;
Quoted 9 lines · jamal
Quoted 4 lines · jamal
What i should say is
if i grabbed the lock explicitly without disabling irqs it wont be much different than what is done today and should always work. No?And to be more explicit, heres a patch using the macros from previous patch. So far tested on 3 NICs.
How many cpu collisions you are seeing?
Did I understand you right, that you replaced trylock with lock and thus removed collision handling and got better results?
Signature · 1 line
Evgeniy Polyakov
Quoted 2 lines · Evgeniy Polyakov
How many cpu collisions you are seeing?
On 4 CPUs which were always transmitting very few - there was contention in the range of 100 per million attempts.
Note: it doesnt matter that 4 cpus were busy, this lock is contended at max (for all NAPI drivers i poked into) between two CPUs.
Quoted 2 lines
Did I understand you right, that you replaced trylock with lock and thus removed collision handling and got better results?
Yes, a small one with the 4 CPUs and no irq binding. Note that in the test cases i run, the contention for queue lock was high (since all CPUs were busy processing traffic).
I think as the the number of CPUs go up, this will become more prominent. The choice is between contending for queue lock or this lock. One lock is contended by max of two cpus, the other by N cpus. As N goes up, you want to have more mercy on the one that is contended by N cpus. Did that make sense?
cheers,
jamal
Quoted 11 lines · jamal
Quoted 2 lines
Did I understand you right, that you replaced trylock with lock and thus removed collision handling and got better results?
Yes, a small one with the 4 CPUs and no irq binding. Note that in the test cases i run, the contention for queue lock was high (since all CPUs were busy processing traffic).
I think as the the number of CPUs go up, this will become more prominent. The choice is between contending for queue lock or this lock. One lock is contended by max of two cpus, the other by N cpus. As N goes up, you want to have more mercy on the one that is contended by N cpus. Did that make sense?
I think if number of cpus grows and there is no interupt binding, system will not scale very well anyway, but your description makes sense, thanks.
Signature · 1 line
Evgeniy Polyakov
David Miller
maintainerFrom: jamal <[email protected]>
Date: Sun, 16 Sep 2007 16:41:24 -0400
Quoted 5 lines
Ok, maybe i am thinking too hard with that patch, so help me out:-> When i looked at that code path as it is today: i felt the softirq could be interupted on the same CPU it is running on while it already grabbed that tx lock (if the trylock succeeds) and that the hardirq code when attempting to grab the lock would result in a deadlock.
Hardirq should never try to grab the netif_tx_lock(), it is only for base and softirq context.
Any hardirq context code taking that lock needs to be fixed. We could assert this if we don't already.
It's the only way that it works that we can invoke ->hard_start_xmit() with interrupts fully enabled.
I notice that your patch bypasses the LLTX logic (I think) and this isn't kosher, it might introduce deadlocks or similar as when we are doing LLTX the driver determines the locking and IRQ context semantics.
Quoted 5 lines · David Miller
Hardirq should never try to grab the
netif_tx_lock(), it is only for base and softirq context.Any hardirq context code taking that lock needs to be fixed. We could assert this if we don't already.
I snooped around it looks pretty clean; An assertion wont hurt, but people will find _very quickly_ it hurts when testing a driver if they did it wrong;->
Quoted 7 lines
It's the only way that it works that we can invoke ->hard_start_xmit() with interrupts fully enabled.
I notice that your patch bypasses the LLTX logic (I think) and this isn't kosher, it might introduce deadlocks or similar as when we are doing LLTX the driver determines the locking and IRQ context semantics.
Nothing much has changed from what it was before.
The only difference is we let go of the queue lock before grabbing the tx lock which never mattered for LLTX.
Once we grab the tx lock it is the same logic and so far is working well on both tg3 and e1000 (which is LLTX).
I will continue to retest with net-2.6.24 once you complete rebasing and look around to see if anyone maybe affected.
cheers,
jamal
Quoted 7 lines · jamal
Nothing much has changed from what it was before.
The only difference is we let go of the queue lock before grabbing the tx lock which never mattered for LLTX.
Once we grab the tx lock it is the same logic and so far is working well on both tg3 and e1000 (which is LLTX).
I will continue to retest with net-2.6.24 once you complete rebasing and look around to see if anyone maybe affected.
Ok, this is looking solid with this mornings tree. Tested on a dual core xeon with e1000 (LLTX) and a dual core opteron with tg3 (non-LLTX). About 100 million packets from udp full throttle on all 4 cpus; i tried pulling cables etc while doing this to generate extrenous interupts and didnt see any issues.
Shall i submit the patch?
cheers,
jamal
David Miller
maintainerFrom: jamal <[email protected]>
Date: Wed, 19 Sep 2007 09:33:52 -0400
Quoted 10 lines · jamal
Quoted 8 lines
Nothing much has changed from what it was before.
The only difference is we let go of the queue lock before grabbing the tx lock which never mattered for LLTX.
Once we grab the tx lock it is the same logic and so far is working well on both tg3 and e1000 (which is LLTX).
I will continue to retest with net-2.6.24 once you complete rebasing and look around to see if anyone maybe affected.
Quoted 7 lines
Ok, this is looking solid with this mornings tree. Tested on a dual core xeon with e1000 (LLTX) and a dual core opteron with tg3 (non-LLTX). About 100 million packets from udp full throttle on all 4 cpus; i tried pulling cables etc while doing this to generate extrenous interupts and didnt see any issues.
Shall i submit the patch?
Sure, along with a description as to why you want to make this change.
I still don't understand the impetus. :)
Quoted 2 lines · David Miller
Sure, along with a description as to why you want to make this change.
Will do. And if you feel that i should sit on it a little more i can do that too. The good news is it doesnt make things any worse than they already are and infact shows a slight improvement in a 4 cpu machine.
Quoted 1 line
I still don't understand the impetus. :)
I will like to use the effect later on to improve batching.
cheers,
jamal
Ok, this is from the net-2.6.24 of about an hour ago.
cheers,
jamal
[NET_SCHED] explict hold dev tx lock
For N cpus, with full throttle traffic on all N CPUs, funneling traffic to the same ethernet device, the devices queue lock is contended by all N CPUs constantly. The TX lock is only contended by a max of 2 CPUS. In the current mode of operation, after all the work of entering the dequeue region, we may endup aborting the path if we are unable to get the tx lock and go back to contend for the queue lock. As N goes up, this gets worse.
The changes in this patch result in a small increase in performance with a 4CPU (2xdual-core) with no irq binding. Both e1000 and tg3 showed similar behavior;
---
commit 5973a9734a93903e7c3547dbe19addc42600240f
tree a57bd488e200fdaa3dfef7f27de52cbe02238dd3
parent 31b9dd879cd9fd254484de2950341acdcb62680f
author Jamal Hadi Salim <[email protected]> Wed, 19 Sep 2007 22:29:16 -0400 committer Jamal Hadi Salim <[email protected]> Wed, 19 Sep 2007 22:29:16 -0400
net/sched/sch_generic.c | 19 ++----------------- 1 files changed, 2 insertions(+), 17 deletions(-)
net/sched/sch_generic.c +2 -17
@@ -134,34 +134,19 @@ static inline int qdisc_restart(struct net_device *dev) {struct Qdisc *q = dev->qdisc;struct sk_buff *skb; -unsigned lockless;int ret;/* Dequeue packet */ if (unlikely ((skb =dev_dequeue_skb (dev, q)) ==NULL ))return 0 ; -/* -* When the driver has LLTX set, it does its own locking in -* start_xmit. These checks are worth it because even uncongested -* locks can be quite expensive. The driver can do a trylock, as -* is being done here; in case of lock contention it should return -* NETDEV_TX_LOCKED and the packet will be requeued. -*/ - lockless = (dev->features &NETIF_F_LLTX ); - -if (!lockless && !netif_tx_trylock (dev)) { -/* Another CPU grabbed the driver tx lock */ -return handle_dev_cpu_collision (skb, dev, q); - }/* And release queue */ spin_unlock (&dev->queue_lock); +HARD_TX_LOCK (dev,smp_processor_id ()); ret =dev_hard_start_xmit (skb, dev); - -if (!lockless) -netif_tx_unlock (dev); +HARD_TX_UNLOCK (dev);spin_lock (&dev->queue_lock); q = dev->qdisc;
From: jamal <[email protected]>
Date: Wed, 19 Sep 2007 22:43:03 -0400
Quoted 10 lines
[
NET_SCHED] explict hold dev tx lockFor N cpus, with full throttle traffic on all N CPUs, funneling traffic to the same ethernet device, the devices queue lock is contended by all N CPUs constantly. The TX lock is only contended by a max of 2 CPUS. In the current mode of operation, after all the work of entering the dequeue region, we may endup aborting the path if we are unable to get the tx lock and go back to contend for the queue lock. As N goes up, this gets worse.
Quoted 5 lines
The changes in this patch result in a small increase in performance with a 4CPU (2xdual-core) with no irq binding. Both e1000 and tg3 showed similar behavior;
Signed-off-by Jamal Hadi Salim <[email protected]>
I've applied this to net-2.6.24, although I want to study more deeply the implications of this change myself at some point :)
Quoted 2 lines · David Miller
I've applied this to net-2.6.24, although I want to study more deeply the implications of this change myself at some point :)
sounds reasonable. Ive done a lot of testing with my 2-3 NIC variants; ive cced whoever i thought was a stakeholder and they ALL seemed silently-happy-joyous (wink;->), so letting more people go at it is the best thing to do. I will be watching that space.
cheers,
jamal
Quoted 11 lines · jamal
[
NET_SCHED] explict hold dev tx lockFor N cpus, with full throttle traffic on all N CPUs, funneling traffic to the same ethernet device, the devices queue lock is contended by all N CPUs constantly. The TX lock is only contended by a max of 2 CPUS. In the current mode of operation, after all the work of entering the dequeue region, we may endup aborting the path if we are unable to get the tx lock and go back to contend for the queue lock. As N goes up, this gets worse.
Quoted 3 lines
The changes in this patch result in a small increase in performance with a 4CPU (2xdual-core) with no irq binding. Both e1000 and tg3 showed similar behavior;
OK, after waking up a bit more I now have another question :)
Both of the drivers you've tested here are special. Firstly e1000 is lockless so there is no contention here at all. On the other hand tg3 doesn't take the TX lock on the clean-up path unless the queue has been stopped.
In other words both drivers only take the TX lock on xmit so this patch makes very little difference to them.
What I'm worried about is would we see worse behaviour with drivers that do all their TX clean-up with the TX lock held (which would cause qdisc_restart to spin while this is happening)?
Cheers,
Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txtQuoted 2 lines · Herbert Xu
OK, after waking up a bit more
me too;->
Quoted 2 lines
What I'm worried about is would we see worse behaviour with drivers that do all their TX clean-up with the TX lock held
Good point Herbert.
When i looked around i only found one driver that behaved like that; some IBM mainframe one from the looks of it. That driver did a lot of other obscure things (i think they maintain their own napi calls etc), so it didnt worry me very much.
IIRC, I think it could be fixed to do what tg3 and relatives like bnx do (I really like the approach) - just didnt have the time to chase it. There are a _lot_ more drivers that have no respect for netif_tx_lock and implement their own locking down in the driver. Those already suffer from the phenomena you describe whether TX_LOCK is held or not.
Quoted 1 line
(which would cause
qdisc_restartto spin while this is happening)?
Yes, with such a driver, we spin in the worst case. But that provides opportunities for optimization of a driver behaving that way; two approaches off top of my head:
a) we could prune the tx descriptors on the the tx path since it is safe to do so and reduce the amount of time spent doing such work in the napi poll
or
b) Have the napi side do a trylock (sort of what the e1000 attempts to do) and reschedule the poll to retry.
#b is fair because the cost of a queue_lock goes up as the number of cpus goes up (which is the case i was optimizing for). The cost of tx lock is contention by two cpus in worst case. Thoughts?
cheers,
jamal