net, skbuff: do not prefer skb allocation fails early
diff mbox series

Message ID alpine.DEB.2.21.1901021300420.62853@chino.kir.corp.google.com
State Accepted
Delegated to: David Miller
Headers show
Series
  • net, skbuff: do not prefer skb allocation fails early
Related show

Commit Message

David Rientjes Jan. 2, 2019, 9:01 p.m. UTC
Commit dcda9b04713c ("mm, tree wide: replace __GFP_REPEAT by 
__GFP_RETRY_MAYFAIL with more useful semantic") replaced __GFP_REPEAT in
alloc_skb_with_frags() with __GFP_RETRY_MAYFAIL when the allocation may
directly reclaim.

The previous behavior would require reclaim up to 1 << order pages for
skb aligned header_len of order > PAGE_ALLOC_COSTLY_ORDER before failing,
otherwise the allocations in alloc_skb() would loop in the page allocator
looking for memory.  __GFP_RETRY_MAYFAIL makes both allocations failable
under memory pressure, including for the HEAD allocation.

This can cause, among many other things, write() to fail with ENOTCONN
during RPC when under memory pressure.  

These allocations should succeed as they did previous to dcda9b04713c
even if it requires calling the oom killer and additional looping in the
page allocator to find memory.  There is no way to specify the previous
behavior of __GFP_REPEAT, but it's unlikely to be necessary since the
previous behavior only guaranteed that 1 << order pages would be reclaimed
before failing for order > PAGE_ALLOC_COSTLY_ORDER.  That reclaim is not
guaranteed to be contiguous memory, so repeating for such large orders is
usually not beneficial.

Removing the setting of __GFP_RETRY_MAYFAIL to restore the previous
behavior, specifically not allowing alloc_skb() to fail for small orders
and oom kill if necessary rather than allowing RPCs to fail.

Fixes: dcda9b04713c ("mm, tree wide: replace __GFP_REPEAT by
__GFP_RETRY_MAYFAIL with more useful semantic") 
Signed-off-by: David Rientjes <rientjes@google.com>
---
 net/core/skbuff.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Eric Dumazet Jan. 3, 2019, 10:29 a.m. UTC | #1
On 01/02/2019 01:01 PM, David Rientjes wrote:
> Commit dcda9b04713c ("mm, tree wide: replace __GFP_REPEAT by 
> __GFP_RETRY_MAYFAIL with more useful semantic") replaced __GFP_REPEAT in
> alloc_skb_with_frags() with __GFP_RETRY_MAYFAIL when the allocation may
> directly reclaim.
> 
> The previous behavior would require reclaim up to 1 << order pages for
> skb aligned header_len of order > PAGE_ALLOC_COSTLY_ORDER before failing,
> otherwise the allocations in alloc_skb() would loop in the page allocator
> looking for memory.  __GFP_RETRY_MAYFAIL makes both allocations failable
> under memory pressure, including for the HEAD allocation.
> 
> This can cause, among many other things, write() to fail with ENOTCONN
> during RPC when under memory pressure.  
> 
> These allocations should succeed as they did previous to dcda9b04713c
> even if it requires calling the oom killer and additional looping in the
> page allocator to find memory.  There is no way to specify the previous
> behavior of __GFP_REPEAT, but it's unlikely to be necessary since the
> previous behavior only guaranteed that 1 << order pages would be reclaimed
> before failing for order > PAGE_ALLOC_COSTLY_ORDER.  That reclaim is not
> guaranteed to be contiguous memory, so repeating for such large orders is
> usually not beneficial.
> 
> Removing the setting of __GFP_RETRY_MAYFAIL to restore the previous
> behavior, specifically not allowing alloc_skb() to fail for small orders
> and oom kill if necessary rather than allowing RPCs to fail.
> 
> Fixes: dcda9b04713c ("mm, tree wide: replace __GFP_REPEAT by
> __GFP_RETRY_MAYFAIL with more useful semantic") 
> Signed-off-by: David Rientjes <rientjes@google.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks David.
David Miller Jan. 4, 2019, 8:54 p.m. UTC | #2
From: David Rientjes <rientjes@google.com>
Date: Wed, 2 Jan 2019 13:01:43 -0800 (PST)

> Commit dcda9b04713c ("mm, tree wide: replace __GFP_REPEAT by 
> __GFP_RETRY_MAYFAIL with more useful semantic") replaced __GFP_REPEAT in
> alloc_skb_with_frags() with __GFP_RETRY_MAYFAIL when the allocation may
> directly reclaim.
> 
> The previous behavior would require reclaim up to 1 << order pages for
> skb aligned header_len of order > PAGE_ALLOC_COSTLY_ORDER before failing,
> otherwise the allocations in alloc_skb() would loop in the page allocator
> looking for memory.  __GFP_RETRY_MAYFAIL makes both allocations failable
> under memory pressure, including for the HEAD allocation.
> 
> This can cause, among many other things, write() to fail with ENOTCONN
> during RPC when under memory pressure.  
> 
> These allocations should succeed as they did previous to dcda9b04713c
> even if it requires calling the oom killer and additional looping in the
> page allocator to find memory.  There is no way to specify the previous
> behavior of __GFP_REPEAT, but it's unlikely to be necessary since the
> previous behavior only guaranteed that 1 << order pages would be reclaimed
> before failing for order > PAGE_ALLOC_COSTLY_ORDER.  That reclaim is not
> guaranteed to be contiguous memory, so repeating for such large orders is
> usually not beneficial.
> 
> Removing the setting of __GFP_RETRY_MAYFAIL to restore the previous
> behavior, specifically not allowing alloc_skb() to fail for small orders
> and oom kill if necessary rather than allowing RPCs to fail.
> 
> Fixes: dcda9b04713c ("mm, tree wide: replace __GFP_REPEAT by
> __GFP_RETRY_MAYFAIL with more useful semantic") 
> Signed-off-by: David Rientjes <rientjes@google.com>

Applied and queued up for -stable.

Patch
diff mbox series

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5270,7 +5270,6 @@  struct sk_buff *alloc_skb_with_frags(unsigned long header_len,
 	unsigned long chunk;
 	struct sk_buff *skb;
 	struct page *page;
-	gfp_t gfp_head;
 	int i;
 
 	*errcode = -EMSGSIZE;
@@ -5280,12 +5279,8 @@  struct sk_buff *alloc_skb_with_frags(unsigned long header_len,
 	if (npages > MAX_SKB_FRAGS)
 		return NULL;
 
-	gfp_head = gfp_mask;
-	if (gfp_head & __GFP_DIRECT_RECLAIM)
-		gfp_head |= __GFP_RETRY_MAYFAIL;
-
 	*errcode = -ENOBUFS;
-	skb = alloc_skb(header_len, gfp_head);
+	skb = alloc_skb(header_len, gfp_mask);
 	if (!skb)
 		return NULL;