* [PATCH] acenic - don't spin in hard_start_xmit when ring fills
@ 2004-09-16 23:17 Stephen Hemminger
2004-09-16 23:22 ` David S. Miller
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2004-09-16 23:17 UTC (permalink / raw)
To: Jes Sorensen, Jeff Garzik; +Cc: netdev
Running performance tests on the acenic, I noticed that the driver
spins when the transmit ring gets full. This might have been okay when
CPU's were slower, but it isn't the right thing to do. Better to
return TX_BUSY and let network scheduling handle it.
Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
diff -Nru a/drivers/net/acenic.c b/drivers/net/acenic.c
--- a/drivers/net/acenic.c 2004-09-16 16:17:09 -07:00
+++ b/drivers/net/acenic.c 2004-09-16 16:17:09 -07:00
@@ -2517,11 +2517,10 @@
struct tx_desc *desc;
u32 idx, flagsize;
-restart:
idx = ap->tx_prd;
if (tx_ring_full(ap, ap->tx_ret_csm, idx))
- goto overflow;
+ return NETDEV_TX_BUSY;
#if MAX_SKB_FRAGS
if (!skb_shinfo(skb)->nr_frags)
@@ -2625,27 +2624,7 @@
}
dev->trans_start = jiffies;
- return 0;
-
-overflow:
- /*
- * This race condition is unavoidable with lock-free drivers.
- * We wake up the queue _before_ tx_prd is advanced, so that we can
- * enter hard_start_xmit too early, while tx ring still looks closed.
- * This happens ~1-4 times per 100000 packets, so that we can allow
- * to loop syncing to other CPU. Probably, we need an additional
- * wmb() in ace_tx_intr as well.
- *
- * Note that this race is relieved by reserving one more entry
- * in tx ring than it is necessary (see original non-SG driver).
- * However, with SG we need to reserve 2*MAX_SKB_FRAGS+1, which
- * is already overkill.
- *
- * Alternative is to return with 1 not throttling queue. In this
- * case loop becomes longer, no more useful effects.
- */
- barrier();
- goto restart;
+ return NETDEV_TX_OK;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] acenic - don't spin in hard_start_xmit when ring fills
2004-09-16 23:17 [PATCH] acenic - don't spin in hard_start_xmit when ring fills Stephen Hemminger
@ 2004-09-16 23:22 ` David S. Miller
2004-09-16 23:42 ` Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: David S. Miller @ 2004-09-16 23:22 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: jes, jgarzik, netdev
On Thu, 16 Sep 2004 16:17:53 -0700
Stephen Hemminger <shemminger@osdl.org> wrote:
> Running performance tests on the acenic, I noticed that the driver
> spins when the transmit ring gets full. This might have been okay when
> CPU's were slower, but it isn't the right thing to do. Better to
> return TX_BUSY and let network scheduling handle it.
Acenic does completely lockless processing, are you sure you didn't
add a race or bug?
Also, NETDEV_TX_BUSY is not a valid return value for non-LLTX drivers.
>From Documentation/networking/netdevices.txt:
o NETDEV_TX_LOCKED Locking failed, please retry quickly.
Only valid when NETIF_F_LLTX is set.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] acenic - don't spin in hard_start_xmit when ring fills
2004-09-16 23:22 ` David S. Miller
@ 2004-09-16 23:42 ` Stephen Hemminger
2004-09-16 23:50 ` David S. Miller
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2004-09-16 23:42 UTC (permalink / raw)
To: David S. Miller; +Cc: jes, jgarzik, netdev
On Thu, 16 Sep 2004 16:22:50 -0700
"David S. Miller" <davem@davemloft.net> wrote:
> On Thu, 16 Sep 2004 16:17:53 -0700
> Stephen Hemminger <shemminger@osdl.org> wrote:
>
> > Running performance tests on the acenic, I noticed that the driver
> > spins when the transmit ring gets full. This might have been okay when
> > CPU's were slower, but it isn't the right thing to do. Better to
> > return TX_BUSY and let network scheduling handle it.
>
> Acenic does completely lockless processing, are you sure you didn't
> add a race or bug?
NO, the bus just isn't fast enough to keep up with the number of small
packets I am shoving at it.
You got TX_LOCKED and TX_BUSY confused. The problem is drivers that
don't check to see if the last packet sent fills the ring and stop
themselves.
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] acenic - don't spin in hard_start_xmit when ring fills
2004-09-16 23:42 ` Stephen Hemminger
@ 2004-09-16 23:50 ` David S. Miller
2004-09-17 16:02 ` Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: David S. Miller @ 2004-09-16 23:50 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: jes, jgarzik, netdev
On Thu, 16 Sep 2004 16:42:06 -0700
Stephen Hemminger <shemminger@osdl.org> wrote:
> NO, the bus just isn't fast enough to keep up with the number of small
> packets I am shoving at it.
>
> You got TX_LOCKED and TX_BUSY confused. The problem is drivers that
> don't check to see if the last packet sent fills the ring and stop
> themselves.
I understand.
But that still makes this change buggy. I believe the two choices
are:
1) Accept this spinning performance characteristic of the
acenic driver.
or
2) Finally give up on acenic's clever lockless scheme and add
the necessary locking + start/stop tx flow control so it
will never have to return TX_BUSY except in absolutely
catastrophic failure cases.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] acenic - don't spin in hard_start_xmit when ring fills
2004-09-16 23:50 ` David S. Miller
@ 2004-09-17 16:02 ` Stephen Hemminger
2004-09-17 18:31 ` David S. Miller
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2004-09-17 16:02 UTC (permalink / raw)
To: David S. Miller; +Cc: jes, jgarzik, netdev
On Thu, 16 Sep 2004 16:50:42 -0700
"David S. Miller" <davem@davemloft.net> wrote:
> On Thu, 16 Sep 2004 16:42:06 -0700
> Stephen Hemminger <shemminger@osdl.org> wrote:
>
> > NO, the bus just isn't fast enough to keep up with the number of small
> > packets I am shoving at it.
> >
> > You got TX_LOCKED and TX_BUSY confused. The problem is drivers that
> > don't check to see if the last packet sent fills the ring and stop
> > themselves.
>
> I understand.
>
> But that still makes this change buggy. I believe the two choices
> are:
>
> 1) Accept this spinning performance characteristic of the
> acenic driver.
What if there is buggy, hardware that never drains the ring.
It can happen.
> 2) Finally give up on acenic's clever lockless scheme and add
> the necessary locking + start/stop tx flow control so it
> will never have to return TX_BUSY except in absolutely
> catastrophic failure cases.
I'll code up a non-lockless version and see if makes any real difference.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] acenic - don't spin in hard_start_xmit when ring fills
2004-09-17 16:02 ` Stephen Hemminger
@ 2004-09-17 18:31 ` David S. Miller
0 siblings, 0 replies; 6+ messages in thread
From: David S. Miller @ 2004-09-17 18:31 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: jes, jgarzik, netdev
On Fri, 17 Sep 2004 09:02:17 -0700
Stephen Hemminger <shemminger@osdl.org> wrote:
> On Thu, 16 Sep 2004 16:50:42 -0700
> "David S. Miller" <davem@davemloft.net> wrote:
>
> > 1) Accept this spinning performance characteristic of the
> > acenic driver.
>
> What if there is buggy, hardware that never drains the ring.
> It can happen.
You're preaching to the choir :-) I've been bugging Alexey about
this aspect of the Acenic driver since day one.
> > 2) Finally give up on acenic's clever lockless scheme and add
> > the necessary locking + start/stop tx flow control so it
> > will never have to return TX_BUSY except in absolutely
> > catastrophic failure cases.
>
> I'll code up a non-lockless version and see if makes any real difference.
Let me know how it goes.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-09-17 18:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-09-16 23:17 [PATCH] acenic - don't spin in hard_start_xmit when ring fills Stephen Hemminger
2004-09-16 23:22 ` David S. Miller
2004-09-16 23:42 ` Stephen Hemminger
2004-09-16 23:50 ` David S. Miller
2004-09-17 16:02 ` Stephen Hemminger
2004-09-17 18:31 ` 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).