Re: [PATCH] ibm_newemac: Fixes memory leak in ibm_newemac ethernet driver

[SathyaNarayanan <sathyan@teamf1.com>]

[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]

Hi Benn,

               Please find my comments below.

On Tue, Jun 24, 2008 at 4:51 AM, Benjamin Herrenschmidt <
benh@kernel.crashing.org> wrote:

> On Mon, 2008-06-23 at 14:55 +0200, Stefan Roese wrote:
> > From: Sathya Narayanan <sathyan@teamf1.com>
> >
> > This patch addresses the memory leak happenning in drivers transmit queue
> > under heavy load condition. Once the transmit queue becomes full, driver
> > does an automatic wrapup of queue. During which the untransmitted SKB's
> are
> > lost without getting freed up.
>
> This would be a bug. We should stop the queue when full instead.


Actually the meachanism of stopping the queue and starting it is already
there.  But even then due to some sync issue between the poll routine and
xmit, we were resulted in using the slots of skb which was not actually got
freed before.
I agree this could a bug , Since its not is not clear why buffers are not
getting transferred timely?. But to handle this we should have a work around
otherwise system may go out of memory. If we go for stopping the queue in
these scenario also ( Where a unfreed skbs slot has been assigned  to
another ), Then kernel may call tx timeout, And reset the driver. In that
case handelling this special case here could lead us better performance as
compared to stopping the queue
Let me know your comments.

>
>
> > Signed-off-by: Sathya Narayanan <sathyan@teamf1.com>
> > Signed-off-by: Stefan Roese <sr@denx.de>
> > ---
> >  drivers/net/ibm_newemac/core.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/net/ibm_newemac/core.c
> b/drivers/net/ibm_newemac/core.c
> > index aa407b2..ee868b6 100644
> > --- a/drivers/net/ibm_newemac/core.c
> > +++ b/drivers/net/ibm_newemac/core.c
> > @@ -1328,6 +1328,13 @@ static int emac_start_xmit(struct sk_buff *skb,
> struct net_device *ndev)
> >
> >       DBG2(dev, "xmit(%u) %d" NL, len, slot);
> >
> > +     if (dev->tx_skb[slot] && dev->tx_desc[slot].data_ptr) {
> > +             dev_kfree_skb(dev->tx_skb[slot]);
> > +             dev->tx_skb[slot] = NULL;
> > +             dev->tx_cnt--;
> > +             ++dev->estats.tx_dropped;
> > +     }
> > +
> >       dev->tx_skb[slot] = skb;
> >       dev->tx_desc[slot].data_ptr = dma_map_single(&dev->ofdev->dev,
> >                                                    skb->data, len,
>
>

[-- Attachment #2: Type: text/html, Size: 3603 bytes --]