* [PATCH] Do less atomic count changes in dev_queue_xmit
@ 2004-09-04 13:54 Andi Kleen
2004-09-05 22:03 ` Herbert Xu
0 siblings, 1 reply; 4+ messages in thread
From: Andi Kleen @ 2004-09-04 13:54 UTC (permalink / raw)
To: davem, netdev
Do a single local_bh_disable and a single local_bh_enable instead of changing
the atomic count all the time in dev_queue_xmit.
Should mostly benefit preemptible kernels, but others see some
small improvements too.
diff -u linux-2.6.8/net/core/dev.c-o linux-2.6.8/net/core/dev.c
--- linux-2.6.8/net/core/dev.c-o 2004-09-04 13:10:47.000000000 +0000
+++ linux-2.6.8/net/core/dev.c 2004-09-04 13:47:16.765722813 +0000
@@ -1249,14 +1249,14 @@
return 0;
}
-#define HARD_TX_LOCK_BH(dev, cpu) { \
+#define HARD_TX_LOCK(dev, cpu) { \
if ((dev->features & NETIF_F_LLTX) == 0) { \
spin_lock_bh(&dev->xmit_lock); \
dev->xmit_lock_owner = cpu; \
} \
}
-#define HARD_TX_UNLOCK_BH(dev) { \
+#define HARD_TX_UNLOCK(dev) { \
if ((dev->features & NETIF_F_LLTX) == 0) { \
dev->xmit_lock_owner = -1; \
spin_unlock_bh(&dev->xmit_lock); \
@@ -1313,7 +1313,12 @@
if (skb_checksum_help(&skb, 0))
goto out_kfree_skb;
- rcu_read_lock();
+
+ /* Disable soft irqs for various locks below. Also
+ * stops preemption for RCU.
+ */
+ local_bh_disable();
+
/* Updates of qdisc are serialized by queue_lock.
* The struct Qdisc which is pointed to by qdisc is now a
* rcu structure - it may be accessed without acquiring
@@ -1332,18 +1337,16 @@
#endif
if (q->enqueue) {
/* Grab device queue */
- spin_lock_bh(&dev->queue_lock);
+ spin_lock(&dev->queue_lock);
rc = q->enqueue(skb, q);
qdisc_run(dev);
- spin_unlock_bh(&dev->queue_lock);
- rcu_read_unlock();
+ spin_unlock(&dev->queue_lock);
rc = rc == NET_XMIT_BYPASS ? NET_XMIT_SUCCESS : rc;
goto out;
}
- rcu_read_unlock();
/* The device has no queue. Common case for software devices:
loopback, all the sorts of tunnels...
@@ -1358,12 +1361,11 @@
Either shot noqueue qdisc, it is even simpler 8)
*/
if (dev->flags & IFF_UP) {
- int cpu = get_cpu();
+ int cpu = smp_processor_id(); /* ok because BHs are off */
if (dev->xmit_lock_owner != cpu) {
- HARD_TX_LOCK_BH(dev, cpu);
- put_cpu();
+ HARD_TX_LOCK(dev, cpu);
if (!netif_queue_stopped(dev)) {
if (netdev_nit)
@@ -1371,17 +1373,16 @@
rc = 0;
if (!dev->hard_start_xmit(skb, dev)) {
- HARD_TX_UNLOCK_BH(dev);
+ HARD_TX_UNLOCK(dev);
goto out;
}
}
- HARD_TX_UNLOCK_BH(dev);
+ HARD_TX_UNLOCK(dev);
if (net_ratelimit())
printk(KERN_CRIT "Virtual device %s asks to "
"queue packet!\n", dev->name);
goto out_enetdown;
} else {
- put_cpu();
/* Recursion is detected! It is possible,
* unfortunately */
if (net_ratelimit())
@@ -1394,6 +1395,7 @@
out_kfree_skb:
kfree_skb(skb);
out:
+ local_bh_enable();
return rc;
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Do less atomic count changes in dev_queue_xmit
2004-09-04 13:54 [PATCH] Do less atomic count changes in dev_queue_xmit Andi Kleen
@ 2004-09-05 22:03 ` Herbert Xu
2004-09-07 11:56 ` Andi Kleen
0 siblings, 1 reply; 4+ messages in thread
From: Herbert Xu @ 2004-09-05 22:03 UTC (permalink / raw)
To: Andi Kleen; +Cc: davem, netdev, akepner
On Sat, Sep 04, 2004 at 01:54:39PM +0000, Andi Kleen wrote:
>
> diff -u linux-2.6.8/net/core/dev.c-o linux-2.6.8/net/core/dev.c
> --- linux-2.6.8/net/core/dev.c-o 2004-09-04 13:10:47.000000000 +0000
> +++ linux-2.6.8/net/core/dev.c 2004-09-04 13:47:16.765722813 +0000
> @@ -1249,14 +1249,14 @@
> return 0;
> }
>
> -#define HARD_TX_LOCK_BH(dev, cpu) { \
> +#define HARD_TX_LOCK(dev, cpu) { \
> if ((dev->features & NETIF_F_LLTX) == 0) { \
> spin_lock_bh(&dev->xmit_lock); \
You can remove the _bh here as well.
> @@ -1358,12 +1361,11 @@
> Either shot noqueue qdisc, it is even simpler 8)
> */
> if (dev->flags & IFF_UP) {
> - int cpu = get_cpu();
> + int cpu = smp_processor_id(); /* ok because BHs are off */
Hmm this means that the loopback xmit function will now execute with
BH/preempt turned off. Is this what we want?
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Do less atomic count changes in dev_queue_xmit
2004-09-05 22:03 ` Herbert Xu
@ 2004-09-07 11:56 ` Andi Kleen
2004-09-07 20:52 ` David S. Miller
0 siblings, 1 reply; 4+ messages in thread
From: Andi Kleen @ 2004-09-07 11:56 UTC (permalink / raw)
To: Herbert Xu; +Cc: Andi Kleen, davem, netdev, akepner
On Mon, Sep 06, 2004 at 08:03:41AM +1000, Herbert Xu wrote:
> On Sat, Sep 04, 2004 at 01:54:39PM +0000, Andi Kleen wrote:
> >
> > diff -u linux-2.6.8/net/core/dev.c-o linux-2.6.8/net/core/dev.c
> > --- linux-2.6.8/net/core/dev.c-o 2004-09-04 13:10:47.000000000 +0000
> > +++ linux-2.6.8/net/core/dev.c 2004-09-04 13:47:16.765722813 +0000
> > @@ -1249,14 +1249,14 @@
> > return 0;
> > }
> >
> > -#define HARD_TX_LOCK_BH(dev, cpu) { \
> > +#define HARD_TX_LOCK(dev, cpu) { \
> > if ((dev->features & NETIF_F_LLTX) == 0) { \
> > spin_lock_bh(&dev->xmit_lock); \
>
> You can remove the _bh here as well.
Ok done.
>
> > @@ -1358,12 +1361,11 @@
> > Either shot noqueue qdisc, it is even simpler 8)
> > */
> > if (dev->flags & IFF_UP) {
> > - int cpu = get_cpu();
> > + int cpu = smp_processor_id(); /* ok because BHs are off */
>
> Hmm this means that the loopback xmit function will now execute with
> BH/preempt turned off. Is this what we want?
I think so yes. It is not that costly.
David, can you please consider this patch, thanks?
-Andi
---------------------------------------------------------------
Streamline atomic count handling in queue xmit fast path.
Only do it once instead of multiple times.
diff -u linux-2.6.8/net/core/dev.c-o linux-2.6.8/net/core/dev.c
--- linux-2.6.8/net/core/dev.c-o 2004-09-04 13:10:47.000000000 +0000
+++ linux-2.6.8/net/core/dev.c 2004-09-07 08:09:52.000000000 +0000
@@ -1249,17 +1249,17 @@
return 0;
}
-#define HARD_TX_LOCK_BH(dev, cpu) { \
+#define HARD_TX_LOCK(dev, cpu) { \
if ((dev->features & NETIF_F_LLTX) == 0) { \
- spin_lock_bh(&dev->xmit_lock); \
+ spin_lock(&dev->xmit_lock); \
dev->xmit_lock_owner = cpu; \
} \
}
-#define HARD_TX_UNLOCK_BH(dev) { \
+#define HARD_TX_UNLOCK(dev) { \
if ((dev->features & NETIF_F_LLTX) == 0) { \
dev->xmit_lock_owner = -1; \
- spin_unlock_bh(&dev->xmit_lock); \
+ spin_unlock(&dev->xmit_lock); \
} \
}
@@ -1313,7 +1313,12 @@
if (skb_checksum_help(&skb, 0))
goto out_kfree_skb;
- rcu_read_lock();
+
+ /* Disable soft irqs for various locks below. Also
+ * stops preemption for RCU.
+ */
+ local_bh_disable();
+
/* Updates of qdisc are serialized by queue_lock.
* The struct Qdisc which is pointed to by qdisc is now a
* rcu structure - it may be accessed without acquiring
@@ -1332,18 +1337,16 @@
#endif
if (q->enqueue) {
/* Grab device queue */
- spin_lock_bh(&dev->queue_lock);
+ spin_lock(&dev->queue_lock);
rc = q->enqueue(skb, q);
qdisc_run(dev);
- spin_unlock_bh(&dev->queue_lock);
- rcu_read_unlock();
+ spin_unlock(&dev->queue_lock);
rc = rc == NET_XMIT_BYPASS ? NET_XMIT_SUCCESS : rc;
goto out;
}
- rcu_read_unlock();
/* The device has no queue. Common case for software devices:
loopback, all the sorts of tunnels...
@@ -1358,12 +1361,11 @@
Either shot noqueue qdisc, it is even simpler 8)
*/
if (dev->flags & IFF_UP) {
- int cpu = get_cpu();
+ int cpu = smp_processor_id(); /* ok because BHs are off */
if (dev->xmit_lock_owner != cpu) {
- HARD_TX_LOCK_BH(dev, cpu);
- put_cpu();
+ HARD_TX_LOCK(dev, cpu);
if (!netif_queue_stopped(dev)) {
if (netdev_nit)
@@ -1371,17 +1373,16 @@
rc = 0;
if (!dev->hard_start_xmit(skb, dev)) {
- HARD_TX_UNLOCK_BH(dev);
+ HARD_TX_UNLOCK(dev);
goto out;
}
}
- HARD_TX_UNLOCK_BH(dev);
+ HARD_TX_UNLOCK(dev);
if (net_ratelimit())
printk(KERN_CRIT "Virtual device %s asks to "
"queue packet!\n", dev->name);
goto out_enetdown;
} else {
- put_cpu();
/* Recursion is detected! It is possible,
* unfortunately */
if (net_ratelimit())
@@ -1394,6 +1395,7 @@
out_kfree_skb:
kfree_skb(skb);
out:
+ local_bh_enable();
return rc;
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Do less atomic count changes in dev_queue_xmit
2004-09-07 11:56 ` Andi Kleen
@ 2004-09-07 20:52 ` David S. Miller
0 siblings, 0 replies; 4+ messages in thread
From: David S. Miller @ 2004-09-07 20:52 UTC (permalink / raw)
To: Andi Kleen; +Cc: herbert, ak, davem, netdev, akepner
On Tue, 7 Sep 2004 13:56:55 +0200
Andi Kleen <ak@suse.de> wrote:
> David, can you please consider this patch, thanks?
Applied, thanks guys.
Andi can you please start providing "Signed-off-by:"
lines with your patches? I can only add my own at
this time, but it makes me feel better if you provide
one too.
Thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2004-09-07 20:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-09-04 13:54 [PATCH] Do less atomic count changes in dev_queue_xmit Andi Kleen
2004-09-05 22:03 ` Herbert Xu
2004-09-07 11:56 ` Andi Kleen
2004-09-07 20:52 ` David S. Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).