MACB: clear transmit buffers properly on TX Underrun
Subsystems (2)
PATCH 1/1 MACB: clear transmit buffers properly on TX Underrun
Hi,
I generated this patch for linux 2.6.24-rc5 and test it on AT91SAM9263 with iperf.
From: Gregory CLEMENT <[email protected]>
Date: Wed, 12 Dec 2007 18:10:14 +0100
Subject: [PATCH] MACB: clear transmit buffers properly on TX Underrun
Initially transmit buffer pointers were only reset. But buffer descriptors were possibly still set as ready, and buffer in upper layer was not freed. This caused driver hang under big load.
Now reset clean properly the buffer descriptor and freed upper layer.
---drivers/net/macb.c | 26 +++++++++++++++++++++++++-
1 files changed, 25 insertions(+), 1 deletions(-)
drivers/net/macb.c +25 -1
@@ -307,9 +307,33 @@ static void macb_tx(struct macb *bp) (unsigned long)status);if (status &MACB_BIT (UND )) { +int i;printk (KERN_ERR "%s: TX underrun, resetting buffers\n" , - bp->dev->name); + bp->dev->name); + + head = bp->tx_head; + +/* free transmit buffer in upper layer*/ +for (tail = bp->tx_tail; tail != head; tail =NEXT_TX (tail)) { +struct ring_info *rp = &bp->tx_skb[tail]; +struct sk_buff *skb = rp->skb; + +BUG_ON (skb ==NULL ); + +rmb (); + +dma_unmap_single (&bp->pdev->dev, rp->mapping, skb->len, +DMA_TO_DEVICE ); + rp->skb =NULL ; +dev_kfree_skb_irq (skb); + } + +/*Mark all the buffer as used to avoid sending a lost buffer*/ +for (i =0 ; i <RX_RING_SIZE ; i++) + bp->tx_ring[i].ctrl =MACB_BIT (TX_USED ); + bp->tx_head = bp->tx_tail =0 ; + }if (!(status &MACB_BIT (COMP )))
Signature · 12 lines
1.5.3.7 -- Gregory CLEMENT Adeneo Adetel Group 2, chemin du Ruisseau 69134 ECULLY - FRANCE Tél. : +33 (0)4 72 18 08 40 - Fax : +33 (0)4 72 18 08 41 www.adetelgroup.com
Quoted 4 lines · On Thu, 13 Dec 2007 08:51:57 +0100 Gregory CLEMENT <[email protected]> wrote:
Hi,
I generated this patch for linux 2.6.24-rc5 and test it on AT91SAM9263 with iperf.
Quoted 8 lines
From: Gregory CLEMENT <[email protected]>
Date: Wed, 12 Dec 2007 18:10:14 +0100
Subject: [PATCH] MACB: clear transmit buffers properly on TX UnderrunInitially transmit buffer pointers were only reset. But buffer descriptors were possibly still set as ready, and buffer in upper layer was not freed. This caused driver hang under big load.
Now reset clean properly the buffer descriptor and freed upper layer.
Nice. I think we want this for 2.6.24.
But the patch is a bit mangled, so I don't think it will apply. Please have a look in Documentation/email-clients.txt for information on how to set up Thunderbird to avoid this.
Quoted 28 lines
Signed-off-by Gregory CLEMENT <[email protected]>--- drivers/net/macb.c | 26 +++++++++++++++++++++++++- 1 files changed, 25 insertions(+), 1 deletions(-)drivers/net/macb.c +13
-1@@ -307,9 +307,33 @@ static void macb_tx(struct macb *bp) (unsigned long)status);if (status &MACB_BIT (UND )) { +int i;printk (KERN_ERR "%s: TX underrun, resetting buffers\n" , - bp->dev->name); + bp->dev->name); + + head = bp->tx_head; + +/* free transmit buffer in upper layer*/ +for (tail = bp->tx_tail; tail != head; tail =NEXT_TX (tail)) { +struct ring_info *rp = &bp->tx_skb[tail]; +struct sk_buff *skb = rp->skb; + +BUG_ON (skb ==NULL ); + +rmb ();
drivers/net/macb.c static void macb_tx(struct macb *bp) · line 333
Quoted 10 lines
drivers/net/macb.c +10
-0+ +dma_unmap_single (&bp->pdev->dev, rp->mapping, skb->len, +DMA_TO_DEVICE ); + rp->skb =NULL ; +dev_kfree_skb_irq (skb); + } + +/*Mark all the buffer as used to avoid sending a lost buffer*/ +for (i =0 ; i <RX_RING_SIZE ; i++) + bp->tx_ring[i].ctrl =MACB_BIT (TX_USED );
That should be TX_RING_SIZE, shouldn't it?
I also think this should be done as part of the previous loop, before they are freed. Having free buffers in the ring sounds dangerous, even if it's only for a short time.
drivers/net/macb.c static void macb_tx(struct macb *bp) · line 335
Quoted 2 lines
drivers/net/macb.c +1
-0+ bp->tx_head = bp->tx_tail =0 ;
Now, I wonder if it would be better to just scan the ring descriptors, find the one that failed and just move the DMA pointer to the next entry. The hardware resets the DMA pointer when an underrun happens, so I think your code will work, but we're losing more packets than strictly necessary. In any case, it's better than the existing code.
Perhaps we need a check in macb_start_xmit() as well to avoid starting a transmission when the ring has just been reset.
Haavard
Quoted 16 lines · Haavard Skinnemoen
Quoted 4 lines · On Thu, 13 Dec 2007 08:51:57 +0100 Gregory CLEMENT <[email protected]> wrote:
Hi,
I generated this patch for linux 2.6.24-rc5 and test it on AT91SAM9263 with iperf.Quoted 8 lines
From: Gregory CLEMENT <[email protected]>
Date: Wed, 12 Dec 2007 18:10:14 +0100
Subject: [PATCH] MACB: clear transmit buffers properly on TX UnderrunInitially transmit buffer pointers were only reset. But buffer descriptors were possibly still set as ready, and buffer in upper layer was not freed. This caused driver hang under big load.
Now reset clean properly the buffer descriptor and freed upper layer.
Quoted 5 lines
Nice. I think we want this for 2.6.24.
But the patch is a bit mangled, so I don't think it will apply. Please have a look in
Documentation/email-clients.txtfor information on how to set up Thunderbird to avoid this.
I read it and apply it. Hope it will be OK as I didn't see any problem in m first mail...
Quoted 33 lines
Quoted 33 lines
Signed-off-by Gregory CLEMENT <[email protected]>--- drivers/net/macb.c | 26 +++++++++++++++++++++++++- 1 files changed, 25 insertions(+), 1 deletions(-)drivers/net/macb.c +17
-1@@ -307,9 +307,33 @@ static void macb_tx(struct macb *bp) (unsigned long)status);if (status &MACB_BIT (UND )) { +int i;printk (KERN_ERR "%s: TX underrun, resetting buffers\n" , - bp->dev->name); + bp->dev->name); + + head = bp->tx_head; + +/* free transmit buffer in upper layer*/ +for (tail = bp->tx_tail; tail != head; tail =NEXT_TX (tail)) { +struct ring_info *rp = &bp->tx_skb[tail]; +struct sk_buff *skb = rp->skb; + +BUG_ON (skb ==NULL ); + +rmb (); + +dma_unmap_single (&bp->pdev->dev, rp->mapping, skb->len, +DMA_TO_DEVICE ); + rp->skb =NULL ;
Quoted 8 lines
drivers/net/macb.c static void macb_tx(struct macb *bp) · line 333
Quoted 6 lines
drivers/net/macb.c +6
-0+dev_kfree_skb_irq (skb); + } + +/*Mark all the buffer as used to avoid sending a lost buffer*/ +for (i =0 ; i <RX_RING_SIZE ; i++) + bp->tx_ring[i].ctrl =MACB_BIT (TX_USED );That should be
TX_RING_SIZE, shouldn't it?
Yes I fixed it
Quoted 4 lines
I also think this should be done as part of the previous loop, before they are freed. Having free buffers in the ring sounds dangerous, even if it's only for a short time.
OK, I moved this loop before the previous loop, as I wanted to be sure that all buffers are marked.
I did it, but I think we don't really need to do it, as when underrun happen, controller stop transmitting, so it won't transmit a free buffer.
Quoted 9 lines
drivers/net/macb.c static void macb_tx(struct macb *bp) · line 335
Quoted 3 lines
drivers/net/macb.c +1
-0+ bp->tx_head = bp->tx_tail =0 ;Now, I wonder if it would be better to just scan the ring descriptors, find the one that failed and just move the DMA pointer to the next entry. The hardware resets the DMA pointer when an underrun happens, so I think your code will work, but we're losing more packets than strictly necessary. In any case, it's better than the existing code.
As hardware reset the DMA pointer, we have to rewrite all the buffer descriptor.
For example if descriptor n°78 failed, and there is descriptor 79 and 80 which are pending. When underrun happen on 78 the hardware reset DMA pointer which will point on descriptor 0. So we have to copy descriptor 79 on 0 and 80 on 1. The other option is to change Transmit Buffer Queue Pointer Register (TBQP), to make it point on descriptor 79, but on the next descriptor having wrapped bit set, the DMA pointer will wrapped on descriptor 79 and not on 0. So I don't think it is a good solution.
Quoted 3 lines
Perhaps we need a check in
macb_start_xmit()as well to avoid starting a transmission when the ring has just been reset.
I joined the patch change along your recommendation, if there is no more error, you can maybe submit it for 2.6.24 release, and other improvement can be done later.
From: Gregory CLEMENT <[email protected]>
Date: Sun, 16 Dec 2007 11:45:03 +0100
Subject: [PATCH] MACB: clear transmit buffers properly on transmit underrun
Initially transmit buffer pointers were only reset. But buffer descriptors were possibly still set as ready, and buffer in upper layer was not freed. This caused driver hang under big load.
Now reset clean properly the buffer descriptor and freed upper layer.
--- drivers/net/macb.c | 27 ++++++++++++++++++++++++++- 1 files changed, 26 insertions(+), 1 deletions(-)
drivers/net/macb.c +26 -1
@@ -307,9 +307,34 @@ static void macb_tx(struct macb *bp) (unsigned long)status);if (status &MACB_BIT (UND )) { +int i;printk (KERN_ERR "%s: TX underrun, resetting buffers\n" , - bp->dev->name); + bp->dev->name); + + head = bp->tx_head; + +/*Mark all the buffer as used to avoid sending a lost buffer*/ +for (i =0 ; i <TX_RING_SIZE ; i++) + bp->tx_ring[i].ctrl =MACB_BIT (TX_USED ); + +/* free transmit buffer in upper layer*/ +for (tail = bp->tx_tail; tail != head; tail =NEXT_TX (tail)) { +struct ring_info *rp = &bp->tx_skb[tail]; +struct sk_buff *skb = rp->skb; + +BUG_ON (skb ==NULL ); + +rmb (); + +dma_unmap_single (&bp->pdev->dev, rp->mapping, skb->len, +DMA_TO_DEVICE ); + rp->skb =NULL ; +dev_kfree_skb_irq (skb); + } + + bp->tx_head = bp->tx_tail =0 ; + }if (!(status &MACB_BIT (COMP )))
Signature · 1 line
1.5.2.5
Quoted 6 lines · On Sun, 16 Dec 2007 12:37:25 +0100 Gregory CLEMENT <[email protected]> wrote:
Quoted 3 lines · Haavard Skinnemoen
But the patch is a bit mangled, so I don't think it will apply. Please have a look in
Documentation/email-clients.txtfor information on how to set up Thunderbird to avoid this.I read it and apply it. Hope it will be OK as I didn't see any problem in m first mail...
Looks good, thanks.
Quoted 9 lines
Quoted 4 lines
I also think this should be done as part of the previous loop, before they are freed. Having free buffers in the ring sounds dangerous, even if it's only for a short time.
OK, I moved this loop before the previous loop, as I wanted to be sure that all buffers are marked.
I did it, but I think we don't really need to do it, as when underrun happen, controller stop transmitting, so it won't transmit a free buffer.
Yes, you're probably right. But it looks better now, I think.
Quoted 12 lines
Quoted 9 lines
Quoted 3 lines
+
bp->tx_head = bp->tx_tail = 0;Now, I wonder if it would be better to just scan the ring descriptors, find the one that failed and just move the DMA pointer to the next entry. The hardware resets the DMA pointer when an underrun happens, so I think your code will work, but we're losing more packets than strictly necessary. In any case, it's better than the existing code.
As hardware reset the DMA pointer, we have to rewrite all the buffer descriptor.
Quoted 7 lines
For example if descriptor n°78 failed, and there is descriptor 79 and 80 which are pending. When underrun happen on 78 the hardware reset DMA pointer which will point on descriptor 0. So we have to copy descriptor 79 on 0 and 80 on 1. The other option is to change Transmit Buffer Queue Pointer Register (TBQP), to make it point on descriptor 79, but on the next descriptor having wrapped bit set, the DMA pointer will wrapped on descriptor 79 and not on 0. So I don't think it is a good solution.
The second option sounds feasible to me. We know how large the ring is, so taking care of the wrapping isn't very expensive.
Quoted 6 lines
Quoted 3 lines
Perhaps we need a check in
macb_start_xmit()as well to avoid starting a transmission when the ring has just been reset.I joined the patch change along your recommendation, if there is no more error, you can maybe submit it for 2.6.24 release, and other improvement can be done later.
Ok, if it works for you, I'll send it upstream; your patch certainly improves things. I think there's still an unresolved race with macb_start_xmit()...but closing it involves more complex modifications to the main tx patch, so it's probably safest to leave it for 2.6.25.
Quoted 19 lines
From: Gregory CLEMENT <[email protected]>
Date: Sun, 16 Dec 2007 11:45:03 +0100
Subject: [PATCH] MACB: clear transmit buffers properly on transmit underrunInitially transmit buffer pointers were only reset. But buffer descriptors were possibly still set as ready, and buffer in upper layer was not freed. This caused driver hang under big load.
Now reset clean properly the buffer descriptor and freed upper layer.Signed-off-by Gregory CLEMENT <[email protected]>--- drivers/net/macb.c | 27 ++++++++++++++++++++++++++- 1 files changed, 26 insertions(+), 1 deletions(-)drivers/net/macb.c +0
-0
drivers/net/macb.c static void macb_tx(struct macb *bp) · line 312
Quoted 7 lines
drivers/net/macb.c +2
-1@@ -307,9 +307,34 @@ static void macb_tx(struct macb *bp) (unsigned long)status);if (status &MACB_BIT (UND )) { +int i;printk (KERN_ERR "%s: TX underrun, resetting buffers\n" , - bp->dev->name); + bp->dev->name);
I'll just kill these extra spaces...
drivers/net/macb.c static void macb_tx(struct macb *bp) · line 337
Quoted 2 lines
drivers/net/macb.c +1
-0bp->tx_head = bp->tx_tail =0 ; +
...and this extra line.
Haavard