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

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

[Stefan Roese]

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.

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,
-- 
1.5.6

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

[Benjamin Herrenschmidt]

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.

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

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

[SathyaNarayanan]

[-- 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 --]

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

[Benjamin Herrenschmidt]

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

Well, if we have a bug, we need to fix it. ie, understand how it is that
the existing mechanism to stop the queue doesn't work, and prevent xmit
from overwriting a non-clear transmit slot (possibly displaying an error
to help us track down the bug).

I'll have to dig a bit, I'll see if I can find some time tomorrow.

Cheers,
Ben.

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

[SathyaNarayanan]

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

On Wed, Jul 2, 2008 at 11:16 AM, Benjamin Herrenschmidt <
benh@kernel.crashing.org> wrote:

>
> >
> > 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.
>
> Well, if we have a bug, we need to fix it. ie, understand how it is that
> the existing mechanism to stop the queue doesn't work, and prevent xmit
> from overwriting a non-clear transmit slot (possibly displaying an error
> to help us track down the bug).
>
> I'll have to dig a bit, I'll see if I can find some time tomorrow.


The reason could be sync issue between poll and xmit. I would like to have
one clarification , Why in the present design no locks has been implemented
to protect the queue from simulatenous access ??

>
>
> Cheers,
> Ben.
>
>
>
>

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