* [PATCH net-next 09/14] tg3: Improve small packet performance
@ 2010-08-02 21:26 Matt Carlson
2010-08-04 22:27 ` Anton Blanchard
0 siblings, 1 reply; 5+ messages in thread
From: Matt Carlson @ 2010-08-02 21:26 UTC (permalink / raw)
To: davem; +Cc: netdev, andy, mcarlson, Michael Chan
smp_mb() inside tg3_tx_avail() is used twice in the normal
tg3_start_xmit() path (see illustration below). The full memory
barrier is only necessary during race conditions with tx completion.
We can speed up the tx path by replacing smp_mb() in tg3_tx_avail()
with a compiler barrier. The compiler barrier is to force the
compiler to fetch the tx_prod and tx_cons from memory.
In the race condition between tg3_start_xmit() and tg3_tx(),
we have the following situation:
tg3_start_xmit() tg3_tx()
if (!tg3_tx_avail())
BUG();
...
if (!tg3_tx_avail())
netif_tx_stop_queue(); update_tx_index();
smp_mb(); smp_mb();
if (tg3_tx_avail()) if (netif_tx_queue_stopped() &&
netif_tx_wake_queue(); tg3_tx_avail())
With smp_mb() removed from tg3_tx_avail(), we need to add smp_mb() to
tg3_start_xmit() as shown above to properly order netif_tx_stop_queue()
and tg3_tx_avail() to check the ring index. If it is not strictly
ordered, the tx queue can be stopped forever.
This improves performance by about 3% with 2 ports running
bi-directional 64-byte packets.
Reviewed-by: Benjamin Li <benli@broadcom•com>
Signed-off-by: Michael Chan <mchan@broadcom•com>
Signed-off-by: Matt Carlson <mcarlson@broadcom•com>
---
drivers/net/tg3.c | 24 +++++++++++++++++++++++-
1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 32e3a3d..820a7dd 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -4386,7 +4386,8 @@ static void tg3_tx_recover(struct tg3 *tp)
static inline u32 tg3_tx_avail(struct tg3_napi *tnapi)
{
- smp_mb();
+ /* Tell compiler to fetch tx indices from memory. */
+ barrier();
return tnapi->tx_pending -
((tnapi->tx_prod - tnapi->tx_cons) & (TG3_TX_RING_SIZE - 1));
}
@@ -5670,6 +5671,13 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
tnapi->tx_prod = entry;
if (unlikely(tg3_tx_avail(tnapi) <= (MAX_SKB_FRAGS + 1))) {
netif_tx_stop_queue(txq);
+
+ /* netif_tx_stop_queue() must be done before checking
+ * checking tx index in tg3_tx_avail() below, because in
+ * tg3_tx(), we update tx index before checking for
+ * netif_tx_queue_stopped().
+ */
+ smp_mb();
if (tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi))
netif_tx_wake_queue(txq);
}
@@ -5715,6 +5723,13 @@ static int tg3_tso_bug(struct tg3 *tp, struct sk_buff *skb)
/* Estimate the number of fragments in the worst case */
if (unlikely(tg3_tx_avail(&tp->napi[0]) <= frag_cnt_est)) {
netif_stop_queue(tp->dev);
+
+ /* netif_tx_stop_queue() must be done before checking
+ * checking tx index in tg3_tx_avail() below, because in
+ * tg3_tx(), we update tx index before checking for
+ * netif_tx_queue_stopped().
+ */
+ smp_mb();
if (tg3_tx_avail(&tp->napi[0]) <= frag_cnt_est)
return NETDEV_TX_BUSY;
@@ -5950,6 +5965,13 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
tnapi->tx_prod = entry;
if (unlikely(tg3_tx_avail(tnapi) <= (MAX_SKB_FRAGS + 1))) {
netif_tx_stop_queue(txq);
+
+ /* netif_tx_stop_queue() must be done before checking
+ * checking tx index in tg3_tx_avail() below, because in
+ * tg3_tx(), we update tx index before checking for
+ * netif_tx_queue_stopped().
+ */
+ smp_mb();
if (tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi))
netif_tx_wake_queue(txq);
}
--
1.7.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net-next 09/14] tg3: Improve small packet performance
2010-08-02 21:26 [PATCH net-next 09/14] tg3: Improve small packet performance Matt Carlson
@ 2010-08-04 22:27 ` Anton Blanchard
2010-08-04 22:46 ` Matt Carlson
2010-08-04 22:47 ` Michael Chan
0 siblings, 2 replies; 5+ messages in thread
From: Anton Blanchard @ 2010-08-04 22:27 UTC (permalink / raw)
To: Matt Carlson; +Cc: davem, netdev, andy, Michael Chan
Hi,
Just saw this go in:
> static inline u32 tg3_tx_avail(struct tg3_napi *tnapi)
> {
> - smp_mb();
> + /* Tell compiler to fetch tx indices from memory. */
> + barrier();
> return tnapi->tx_pending -
> ((tnapi->tx_prod - tnapi->tx_cons) & (TG3_TX_RING_SIZE - 1));
> }
Which worries me. Are we sure we don't need any ordering (eg smp_rmb)?
A compiler barrier does nothing to ensure two loads are ordered.
Anton
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next 09/14] tg3: Improve small packet performance
2010-08-04 22:27 ` Anton Blanchard
@ 2010-08-04 22:46 ` Matt Carlson
2010-08-04 22:47 ` Michael Chan
1 sibling, 0 replies; 5+ messages in thread
From: Matt Carlson @ 2010-08-04 22:46 UTC (permalink / raw)
To: Anton Blanchard
Cc: Matthew Carlson, davem@davemloft•net, netdev@vger•kernel.org,
andy@greyhouse•net, Michael Chan
On Wed, Aug 04, 2010 at 03:27:41PM -0700, Anton Blanchard wrote:
>
> Hi,
>
> Just saw this go in:
>
> > static inline u32 tg3_tx_avail(struct tg3_napi *tnapi)
> > {
> > - smp_mb();
> > + /* Tell compiler to fetch tx indices from memory. */
> > + barrier();
> > return tnapi->tx_pending -
> > ((tnapi->tx_prod - tnapi->tx_cons) & (TG3_TX_RING_SIZE - 1));
> > }
>
> Which worries me. Are we sure we don't need any ordering (eg smp_rmb)?
> A compiler barrier does nothing to ensure two loads are ordered.
>
> Anton
The compiler barrier makes sure the loads stay roughly in the same
location. In the places where the ordering really matters, we have the
memory barrier inlined into the calling code. In the rest of the
places, we can relax the ordering requirement because the final value is
just used as an estimate.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next 09/14] tg3: Improve small packet performance
2010-08-04 22:27 ` Anton Blanchard
2010-08-04 22:46 ` Matt Carlson
@ 2010-08-04 22:47 ` Michael Chan
2010-08-04 23:08 ` Anton Blanchard
1 sibling, 1 reply; 5+ messages in thread
From: Michael Chan @ 2010-08-04 22:47 UTC (permalink / raw)
To: Anton Blanchard
Cc: Matthew Carlson, davem@davemloft•net, netdev@vger•kernel.org,
andy@greyhouse•net
On Wed, 2010-08-04 at 15:27 -0700, Anton Blanchard wrote:
> Hi,
>
> Just saw this go in:
>
> > static inline u32 tg3_tx_avail(struct tg3_napi *tnapi)
> > {
> > - smp_mb();
> > + /* Tell compiler to fetch tx indices from memory. */
> > + barrier();
> > return tnapi->tx_pending -
> > ((tnapi->tx_prod - tnapi->tx_cons) & (TG3_TX_RING_SIZE - 1));
> > }
>
> Which worries me. Are we sure we don't need any ordering (eg smp_rmb)?
> A compiler barrier does nothing to ensure two loads are ordered.
We generally only get an estimate of the available tx ring size when we
call tg3_tx_avail(), so memory barriers are not generally needed. We
put a compiler barrier there to make sure that the compiler will fetch
the tx_prod and tx_cons from memory to give us a better estimate.
In specific cases detailed in the patch description, we do need memory
barriers when we call netif_tx_stop_queue() and then check for the tx
ring. We decided to put memory barriers exactly where they're needed
instead of inside tg3_tx_avail() which is an overkill.
Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next 09/14] tg3: Improve small packet performance
2010-08-04 22:47 ` Michael Chan
@ 2010-08-04 23:08 ` Anton Blanchard
0 siblings, 0 replies; 5+ messages in thread
From: Anton Blanchard @ 2010-08-04 23:08 UTC (permalink / raw)
To: Michael Chan
Cc: Matthew Carlson, davem@davemloft•net, netdev@vger•kernel.org,
andy@greyhouse•net
Hi,
> We generally only get an estimate of the available tx ring size when we
> call tg3_tx_avail(), so memory barriers are not generally needed. We
> put a compiler barrier there to make sure that the compiler will fetch
> the tx_prod and tx_cons from memory to give us a better estimate.
>
> In specific cases detailed in the patch description, we do need memory
> barriers when we call netif_tx_stop_queue() and then check for the tx
> ring. We decided to put memory barriers exactly where they're needed
> instead of inside tg3_tx_avail() which is an overkill.
Thanks Matt and Michael. I was pretty sure you were thinking about
ordering issues but wanted to double check.
Anton
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-08-04 23:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-08-02 21:26 [PATCH net-next 09/14] tg3: Improve small packet performance Matt Carlson
2010-08-04 22:27 ` Anton Blanchard
2010-08-04 22:46 ` Matt Carlson
2010-08-04 22:47 ` Michael Chan
2010-08-04 23:08 ` Anton Blanchard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox