* [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
@ 2016-03-29 4:47 Wei-Ning Huang
2016-03-29 5:31 ` James Cameron
2016-03-29 8:37 ` Kalle Valo
0 siblings, 2 replies; 8+ messages in thread
From: Wei-Ning Huang @ 2016-03-29 4:47 UTC (permalink / raw)
To: linux-wireless
Cc: LKML, Amitkumar Karwar, Nishant Sarmukadam, snanda,
Wei-Ning Huang, kvalo, netdev
"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 <wnhuang@chromium.org>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index b2c839a..c64989c 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -1124,7 +1124,8 @@ static void mwifiex_deaggr_sdio_pkt(struct mwifiex_adapter *adapter,
break;
}
skb_deaggr = mwifiex_alloc_dma_align_buf(pkt_len,
- GFP_KERNEL | GFP_DMA);
+ GFP_KERNEL | GFP_DMA |
+ __GFP_REPEAT);
if (!skb_deaggr)
break;
skb_put(skb_deaggr, pkt_len);
@@ -1374,7 +1375,8 @@ static int mwifiex_sdio_card_to_host_mp_aggr(struct mwifiex_adapter *adapter,
/* copy pkt to deaggr buf */
skb_deaggr = mwifiex_alloc_dma_align_buf(len_arr[pind],
GFP_KERNEL |
- GFP_DMA);
+ GFP_DMA |
+ __GFP_REPEAT);
if (!skb_deaggr) {
mwifiex_dbg(adapter, ERROR, "skb allocation failure\t"
"drop pkt len=%d type=%d\n",
@@ -1416,7 +1418,8 @@ rx_curr_single:
mwifiex_dbg(adapter, INFO, "info: RX: port: %d, rx_len: %d\n",
port, rx_len);
- skb = mwifiex_alloc_dma_align_buf(rx_len, GFP_KERNEL | GFP_DMA);
+ skb = mwifiex_alloc_dma_align_buf(rx_len, GFP_KERNEL | GFP_DMA |
+ __GFP_REPEAT);
if (!skb) {
mwifiex_dbg(adapter, ERROR,
"single skb allocated fail,\t"
@@ -1521,7 +1524,8 @@ static int mwifiex_process_int_status(struct mwifiex_adapter *adapter)
rx_len = (u16) (rx_blocks * MWIFIEX_SDIO_BLOCK_SIZE);
mwifiex_dbg(adapter, INFO, "info: rx_len = %d\n", rx_len);
- skb = mwifiex_alloc_dma_align_buf(rx_len, GFP_KERNEL | GFP_DMA);
+ skb = mwifiex_alloc_dma_align_buf(rx_len, GFP_KERNEL | GFP_DMA |
+ __GFP_REPEAT);
if (!skb)
return -1;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
2016-03-29 4:47 [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call Wei-Ning Huang
@ 2016-03-29 5:31 ` James Cameron
2016-03-29 8:37 ` Kalle Valo
1 sibling, 0 replies; 8+ messages in thread
From: James Cameron @ 2016-03-29 5:31 UTC (permalink / raw)
To: Wei-Ning Huang
Cc: linux-wireless, LKML, Amitkumar Karwar, Nishant Sarmukadam,
snanda, kvalo, netdev
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/
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
2016-03-29 4:47 [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call Wei-Ning Huang
2016-03-29 5:31 ` James Cameron
@ 2016-03-29 8:37 ` Kalle Valo
2016-03-29 9:27 ` Wei-Ning Huang
1 sibling, 1 reply; 8+ messages in thread
From: Kalle Valo @ 2016-03-29 8:37 UTC (permalink / raw)
To: Wei-Ning Huang
Cc: linux-wireless, LKML, Amitkumar Karwar, Nishant Sarmukadam,
snanda, netdev
Wei-Ning Huang <wnhuang@chromium.org> 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 <wnhuang@chromium.org>
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
2016-03-29 8:37 ` Kalle Valo
@ 2016-03-29 9:27 ` Wei-Ning Huang
2016-03-29 10:51 ` Amitkumar Karwar
2016-03-29 12:58 ` Eric Dumazet
0 siblings, 2 replies; 8+ messages in thread
From: Wei-Ning Huang @ 2016-03-29 9:27 UTC (permalink / raw)
To: Kalle Valo
Cc: Linux Wireless, LKML, Amitkumar Karwar, Nishant Sarmukadam,
Sameer Nanda, netdev, Sonny Rao, Douglas Anderson
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 <kvalo@codeaurora.org> wrote:
> Wei-Ning Huang <wnhuang@chromium.org> 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 <wnhuang@chromium.org>
>
> 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 |
wnhuang@google.com | Cell: +886 910-380678
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
2016-03-29 9:27 ` Wei-Ning Huang
@ 2016-03-29 10:51 ` Amitkumar Karwar
2016-03-29 12:58 ` Eric Dumazet
1 sibling, 0 replies; 8+ messages in thread
From: Amitkumar Karwar @ 2016-03-29 10:51 UTC (permalink / raw)
To: Wei-Ning Huang, Kalle Valo
Cc: Linux Wireless, LKML, Nishant Sarmukadam, Sameer Nanda, netdev,
Sonny Rao, Douglas Anderson
> From: Wei-Ning Huang [mailto:wnhuang@google.com]
> Sent: Tuesday, March 29, 2016 2:57 PM
> To: Kalle Valo
> Cc: 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
>
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
2016-03-29 9:27 ` Wei-Ning Huang
2016-03-29 10:51 ` Amitkumar Karwar
@ 2016-03-29 12:58 ` Eric Dumazet
2016-04-05 5:48 ` Amitkumar Karwar
1 sibling, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2016-03-29 12:58 UTC (permalink / raw)
To: Wei-Ning Huang
Cc: Kalle Valo, Linux Wireless, LKML, Amitkumar Karwar,
Nishant Sarmukadam, Sameer Nanda, netdev, Sonny Rao,
Douglas Anderson
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);
^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
2016-03-29 12:58 ` Eric Dumazet
@ 2016-04-05 5:48 ` Amitkumar Karwar
2016-04-05 16:05 ` David Laight
0 siblings, 1 reply; 8+ messages in thread
From: Amitkumar Karwar @ 2016-04-05 5:48 UTC (permalink / raw)
To: Eric Dumazet, Wei-Ning Huang
Cc: Kalle Valo, Linux Wireless, LKML, Nishant Sarmukadam,
Sameer Nanda, netdev, Sonny Rao, Douglas Anderson
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call
2016-04-05 5:48 ` Amitkumar Karwar
@ 2016-04-05 16:05 ` David Laight
0 siblings, 0 replies; 8+ messages in thread
From: David Laight @ 2016-04-05 16:05 UTC (permalink / raw)
To: 'Amitkumar Karwar', Eric Dumazet, Wei-Ning Huang
Cc: Kalle Valo, Linux Wireless, LKML, Nishant Sarmukadam,
Sameer Nanda, netdev, Sonny Rao, Douglas Anderson
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-04-05 16:06 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-29 4:47 [PATCH] mwifiex: add __GFP_REPEAT to skb allocation call Wei-Ning Huang
2016-03-29 5:31 ` James Cameron
2016-03-29 8:37 ` Kalle Valo
2016-03-29 9:27 ` Wei-Ning Huang
2016-03-29 10:51 ` Amitkumar Karwar
2016-03-29 12:58 ` Eric Dumazet
2016-04-05 5:48 ` Amitkumar Karwar
2016-04-05 16:05 ` David Laight
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).