[PATCH] MACB: clear transmit buffers properly on TX Underrun

[PATCH] MACB: clear transmit buffers properly on TX Underrun

[Gregory CLEMENT]

Hi,
I generated this patch for linux 2.6.24-rc5 and test it on AT91SAM9263 
with iperf.

From: Gregory CLEMENT <gclement00@gmail.com>
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.

Signed-off-by: Gregory CLEMENT <gclement00@gmail.com>
---
 drivers/net/macb.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index 047ea7b..2ee1dab 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -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)))
-- 
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

Re: [PATCH] MACB: clear transmit buffers properly on TX Underrun

[Haavard Skinnemoen]

On Thu, 13 Dec 2007 08:51:57 +0100
Gregory CLEMENT <novexz@gmail.com> wrote:

> Hi,
> I generated this patch for linux 2.6.24-rc5 and test it on AT91SAM9263 
> with iperf.
> 
> From: Gregory CLEMENT <gclement00@gmail.com>
> 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.

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.

> Signed-off-by: Gregory CLEMENT <gclement00@gmail.com>
> ---
>  drivers/net/macb.c |   26 +++++++++++++++++++++++++-
>  1 files changed, 25 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> index 047ea7b..2ee1dab 100644
> --- a/drivers/net/macb.c
> +++ b/drivers/net/macb.c
> @@ -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);

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.

> +
>          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

Re: [PATCH] MACB: clear transmit buffers properly on TX Underrun

[Gregory CLEMENT]

Haavard Skinnemoen a écrit :
> On Thu, 13 Dec 2007 08:51:57 +0100
> Gregory CLEMENT <novexz@gmail.com> wrote:
> 
>> Hi,
>> I generated this patch for linux 2.6.24-rc5 and test it on AT91SAM9263 
>> with iperf.
>>
>> From: Gregory CLEMENT <gclement00@gmail.com>
>> 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.
> 
> 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.

I read it and apply it. Hope it will be OK as I didn't see any problem in m first mail...

> 
>> Signed-off-by: Gregory CLEMENT <gclement00@gmail.com>
>> ---
>>  drivers/net/macb.c |   26 +++++++++++++++++++++++++-
>>  1 files changed, 25 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
>> index 047ea7b..2ee1dab 100644
>> --- a/drivers/net/macb.c
>> +++ b/drivers/net/macb.c
>> @@ -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);
> 
> That should be TX_RING_SIZE, shouldn't it?

Yes  I fixed 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.

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.

> 
>> +
>>          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.

> 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 <gclement00@gmail.com>
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.

Signed-off-by: Gregory CLEMENT <gclement00@gmail.com>
---
 drivers/net/macb.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index 047ea7b..e898713 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -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)))
-- 
1.5.2.5

Re: [PATCH] MACB: clear transmit buffers properly on TX Underrun

[Haavard Skinnemoen]

On Sun, 16 Dec 2007 12:37:25 +0100
Gregory CLEMENT <gclement00@gmail.com> wrote:

> Haavard Skinnemoen a écrit :
> > 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.
> 
> 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.

> > 
> > 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.

> > 
> >> +
> >>          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.

The second option sounds feasible to me. We know how large the ring is,
so taking care of the wrapping isn't very expensive.

> > 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.

> From: Gregory CLEMENT <gclement00@gmail.com>
> 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.
> 
> Signed-off-by: Gregory CLEMENT <gclement00@gmail.com>
> ---
>  drivers/net/macb.c |   27 ++++++++++++++++++++++++++-
>  1 files changed, 26 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> index 047ea7b..e898713 100644
> --- a/drivers/net/macb.c
> +++ b/drivers/net/macb.c
> @@ -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...


>  		bp->tx_head = bp->tx_tail = 0;
> +

...and this extra line.

Haavard