From: Amitkumar Karwar <akarwar@marvell.com>
To: Eric Dumazet <eric.dumazet@gmail.com>, Wei-Ning Huang <wnhuang@google.com>
CC: Kalle Valo <kvalo@codeaurora.org>,
        Linux Wireless <linux-wireless@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Nishant Sarmukadam <nishants@marvell.com>,
        Sameer Nanda <snanda@google.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        Sonny Rao <sonnyrao@chromium.org>,
        Douglas Anderson <dianders@chromium.org>
Subject: RE: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
Thread-Topic: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
Thread-Index: AQHRiXYoHVEQswN050uRfddbG1brt59wGbN2gACDFwCAADsQAIAKDmvA
Date: Tue, 5 Apr 2016 05:48:25 +0000
Message-ID: <4fa3762986eb4b848dbe7daf6871d7a8@SC-EXCH04.marvell.com>
References: <1459226840-36287-1-git-send-email-wnhuang@chromium.org>
	 <87vb45brxc.fsf@kamboji.qca.qualcomm.com>
	 <CABicQ-UJ1kniq6m86QcyVZBPznvNexfrUrTk3A_=GmZy69hvCA@mail.gmail.com>
 <1459256320.6473.160.camel@edumazet-glaptop3.roam.corp.google.com>
In-Reply-To: <1459256320.6473.160.camel@edumazet-glaptop3.roam.corp.google.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Transfer-Encoding: 8bit
Content-Length: 2501
Lines: 62

Hi Eric,

Thanks for the comments.

> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Tuesday, March 29, 2016 6:29 PM
> To: Wei-Ning Huang
> Cc: Kalle Valo; Linux Wireless; LKML; Amitkumar Karwar; Nishant
> Sarmukadam; Sameer Nanda; netdev@vger.kernel.org; Sonny Rao; Douglas
> Anderson
> Subject: Re: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
> 
> On Tue, 2016-03-29 at 17:27 +0800, Wei-Ning Huang wrote:
> > Adding some chromium devs to the thread.
> >
> > In, http://lxr.free-electrons.com/source/mm/page_alloc.c#L3152
> >
> > The default mm retry allocation when 'order <=
> > PAGE_ALLOC_COSTLY_ORDER' of gfp_mask contains __GFP_REPEAT.
> > PAGE_ALLOC_COSTLY_ORDER is defined to be 3. On systems with page size
> > = 4K, this means memory compaction and retry is only done when the
> > size of allocation is <= 32K In mwifiex, the allocation size is 64K.
> 
> 
> 
> >  When we have system with
> > memory fragmentation and allocation failed, there will be no retry.
> > This is why we need to add __GFP_REPEAT here to allow the system to
> > perform memory compaction and retry allocation.
> >
> > Maybe Amit@marvell can comment on if this is a good fix on this issue.
> > I'm also aware that marvell is the progress of implementing
> > scatter/gatter for mwifiex, which can also fix the issue.
> 
> Before SG is implemented, you really need to copy incoming frames into
> smallest chunks (to get lowest skb->truesize) and leave the 64KB
> allocated stuff forever in the driver.

We do have a 64KB pre-allocated buffer for receiving Rx data in our driver.

> 
> __GFP_REPEAT wont really solve the issue.
> 
> It seems the problem comes from the fact that the drivers calls
> dev_kfree_skb_any() after calling mwifiex_deaggr_sdio_pkt(), instead of
> recycling this very precious 64KB skb once memory gets fragmented.

Our one time allocated 64k buffer read from firmware contains multiple data chunks. We have a feature called single port aggregation in which firmware attaches an aggregated buffer to single port. So sometimes a single data chunk can exceed 32k. dev_kfree_skb_any() is called to free those data chunks.

> 
> Another problem is that mwifiex_deaggr_sdio_pkt() uses
> mwifiex_alloc_dma_align_buf() with GFP_KERNEL | GFP_DMA
> 
> Really GFP_DMA makes no sense here, since the skb is going to be
> processed by the stack, which has no such requirement.
> 
> Please use normal skb allocations there.

Sure. I will submit a patch for this.

Regards,
Amitkumar