From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from hu-out-0506.google.com (hu-out-0506.google.com [72.14.214.225]) by ozlabs.org (Postfix) with ESMTP id 56F04DDEE1 for ; Wed, 2 Jul 2008 14:45:23 +1000 (EST) Received: by hu-out-0506.google.com with SMTP id 34so991195hue.9 for ; Tue, 01 Jul 2008 21:45:22 -0700 (PDT) Message-ID: <1946a170807012145v6e6d629cvea443f2b8462d708@mail.gmail.com> Date: Wed, 2 Jul 2008 10:15:21 +0530 From: SathyaNarayanan To: benh@kernel.crashing.org Subject: Re: [PATCH] ibm_newemac: Fixes memory leak in ibm_newemac ethernet driver In-Reply-To: <1214263310.8011.280.camel@pasglop> MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="----=_Part_3388_4513222.1214973921901" References: <1214225712-25882-1-git-send-email-sr@denx.de> <1214263310.8011.280.camel@pasglop> Cc: linuxppc-dev@ozlabs.org, Stefan Roese , netdev@vger.kernel.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , ------=_Part_3388_4513222.1214973921901 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline 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 > > > > 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 > > Signed-off-by: Stefan Roese > > --- > > 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, > > ------=_Part_3388_4513222.1214973921901 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline 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,


------=_Part_3388_4513222.1214973921901--