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.228]) by ozlabs.org (Postfix) with ESMTP id 172E1DDF27 for ; Wed, 2 Jul 2008 16:11:43 +1000 (EST) Received: by hu-out-0506.google.com with SMTP id 34so1032522hue.9 for ; Tue, 01 Jul 2008 23:11:41 -0700 (PDT) Message-ID: <1946a170807012311o1595d9d1r477fabc8f4504f7d@mail.gmail.com> Date: Wed, 2 Jul 2008 11:41:41 +0530 From: SathyaNarayanan To: benh@kernel.crashing.org Subject: Re: [PATCH] ibm_newemac: Fixes memory leak in ibm_newemac ethernet driver In-Reply-To: <1214977560.21182.35.camel@pasglop> MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="----=_Part_3515_8103274.1214979101172" References: <1214225712-25882-1-git-send-email-sr@denx.de> <1214263310.8011.280.camel@pasglop> <1946a170807012145v6e6d629cvea443f2b8462d708@mail.gmail.com> <1214977560.21182.35.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_3515_8103274.1214979101172 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline 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. > > > > ------=_Part_3515_8103274.1214979101172 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline

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.




------=_Part_3515_8103274.1214979101172--