RE: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
From: Amitkumar Karwar > Sent: 05 April 2016 06:48 ... > 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. Ah yes, which particular problem does aggregating data into a single buffer solve? David N�r��yb�X��ǧv�^�){.n�+{��*ޕ�,�{ay�ʇڙ�,j��f���h���z��w��� ���j:+v���w�j�mzZ+�ݢj"��!�i
RE: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
Hi Eric, Thanks for the comments. > From: Eric Dumazet [mailto:eric.duma...@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; net...@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
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. __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. 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. diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c index b2c839a..8404db5 100644 --- a/drivers/net/wireless/marvell/mwifiex/sdio.c +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c @@ -1123,8 +1123,8 @@ static void mwifiex_deaggr_sdio_pkt(struct mwifiex_adapter *adapter, __func__, pkt_len, blk_size); break; } - skb_deaggr = mwifiex_alloc_dma_align_buf(pkt_len, -GFP_KERNEL | GFP_DMA); + skb_deaggr = __netdev_alloc_skb_ip_align(NULL, pkt_len, +GFP_KERNEL); if (!skb_deaggr) break; skb_put(skb_deaggr, pkt_len); -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
> From: Wei-Ning Huang [mailto:wnhu...@google.com] > Sent: Tuesday, March 29, 2016 2:57 PM > To: Kalle Valo > Cc: Linux Wireless; LKML; Amitkumar Karwar; Nishant Sarmukadam; Sameer > Nanda; net...@vger.kernel.org; Sonny Rao; Douglas Anderson > Subject: Re: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call > > 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. > > Wei-Ning > This fix would be useful. We have a feature called single port aggregation in which sometimes data received from SDIO interface can be >32k (but less than 64k). This feature improves throughput performance. We are preparing patches for scatter/gather feature. but scatter/gather won't be supported by some platforms. Hence this fix would still be needed. Regards, Amitkumar
Re: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
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. Wei-Ning On Tue, Mar 29, 2016 at 4:37 PM, Kalle Valo wrote: > Wei-Ning Huang writes: > >> "single skb allocation failure" happens when system is under heavy >> memory pressure. Add __GFP_REPEAT to skb allocation call so kernel >> attempts to reclaim pages and retry the allocation. >> >> Signed-off-by: Wei-Ning Huang > > Is this really a proper way to fix the issue? This is the first time I'm > hearing about the flag and there isn't even a single user in > drivers/net. I would like to get confirmation from others that > __GFP_REPEAT is really ok to use in a wireless driver before I can take > this. > > -- > Kalle Valo -- Wei-Ning Huang, 黃偉寧 | Software Engineer, Google Inc., Taiwan | wnhu...@google.com | Cell: +886 910-380678 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
Wei-Ning Huang writes: > "single skb allocation failure" happens when system is under heavy > memory pressure. Add __GFP_REPEAT to skb allocation call so kernel > attempts to reclaim pages and retry the allocation. > > Signed-off-by: Wei-Ning Huang Is this really a proper way to fix the issue? This is the first time I'm hearing about the flag and there isn't even a single user in drivers/net. I would like to get confirmation from others that __GFP_REPEAT is really ok to use in a wireless driver before I can take this. -- Kalle Valo -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
On Tue, Mar 29, 2016 at 12:47:20PM +0800, Wei-Ning Huang wrote: > "single skb allocation failure" happens when system is under heavy > memory pressure. Add __GFP_REPEAT to skb allocation call so kernel > attempts to reclaim pages and retry the allocation. Oh, that's interesting, we're back to this symptom again. Nice to see this fix. Heavy memory pressure on 3.5 caused dev_alloc_skb failure in this driver. Tracked at OLPC as #12694. -- James Cameron http://quozl.netrek.org/ -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html