On Sun, 16 Dec 2007 12:37:25 +0100
Gregory CLEMENT <[EMAIL PROTECTED]> 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 <[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.
>
> Signed-off-by: Gregory CLEMENT <[EMAIL PROTECTED]>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
