diff mbox

[v2] skbuff: Hide GFP_ATOMIC page allocation failures for dropped packets

Message ID 1369599557-22677-1-git-send-email-atomlin@redhat.com
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Aaron Tomlin May 26, 2013, 8:19 p.m. UTC
From: Aaron Tomlin <atomlin@redhat.com>

Since v1:
 - Removed unnecessary parentheses

---8<---

Failed GFP_ATOMIC allocations by the network stack result in dropped
packets, which will be received on a subsequent retransmit, and an
unnecessary, noisy warning with a kernel backtrace.

These warnings are harmless, but they still cause users to panic and
file bug reports over dropped packets. It would be better to hide the
failed allocation warnings and backtraces, and let retransmits handle
dropped packets quietly.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Rik van Riel May 27, 2013, 5:39 p.m. UTC | #1
On 05/26/2013 04:19 PM, atomlin@redhat.com wrote:
> From: Aaron Tomlin <atomlin@redhat.com>
>
> Since v1:
>   - Removed unnecessary parentheses
>
> ---8<---
>
> Failed GFP_ATOMIC allocations by the network stack result in dropped
> packets, which will be received on a subsequent retransmit, and an
> unnecessary, noisy warning with a kernel backtrace.
>
> These warnings are harmless, but they still cause users to panic and
> file bug reports over dropped packets. It would be better to hide the
> failed allocation warnings and backtraces, and let retransmits handle
> dropped packets quietly.

Yes please. Getting memory management bug reports for
dropped network packets got old years ago.  Lets get
rid of those messages.

> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>

Reviewed-by: Rik van Riel <riel@redhat.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov May 27, 2013, 5:56 p.m. UTC | #2
Hello.

On 27-05-2013 0:19, atomlin@redhat.com wrote:

> From: Aaron Tomlin <atomlin@redhat.com>

> Since v1:
>   - Removed unnecessary parentheses

    The "changes since version X" section should typically go after --- 
tearline, else the mainatiner will have to edit your patch before applying.

> ---8<---

> Failed GFP_ATOMIC allocations by the network stack result in dropped
> packets, which will be received on a subsequent retransmit, and an
> unnecessary, noisy warning with a kernel backtrace.

> These warnings are harmless, but they still cause users to panic and
> file bug reports over dropped packets. It would be better to hide the
> failed allocation warnings and backtraces, and let retransmits handle
> dropped packets quietly.

> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet May 27, 2013, 10:25 p.m. UTC | #3
On Mon, 2013-05-27 at 13:39 -0400, Rik van Riel wrote:
> On 05/26/2013 04:19 PM, atomlin@redhat.com wrote:
> > From: Aaron Tomlin <atomlin@redhat.com>
> >
> > Since v1:
> >   - Removed unnecessary parentheses
> >
> > ---8<---
> >
> > Failed GFP_ATOMIC allocations by the network stack result in dropped
> > packets, which will be received on a subsequent retransmit, and an
> > unnecessary, noisy warning with a kernel backtrace.

This claim is wrong, only some protocols deal with retransmits.

> >
> > These warnings are harmless, but they still cause users to panic and
> > file bug reports over dropped packets. It would be better to hide the
> > failed allocation warnings and backtraces, and let retransmits handle
> > dropped packets quietly.
> 
> Yes please. Getting memory management bug reports for
> dropped network packets got old years ago.  Lets get
> rid of those messages.

I am only wondering why this path has anything needing special
attention, over thousands of kmalloc() like call sites in the kernel.

If mm allocation warnings are useless, just make __GFP_NOWARN the
default, and save us thousand of patches (adding the __GFP_NOWARN
everywhere)

Truth is : some network drivers don't deal very well with allocation
errors. mlx4 for example absolutely wants order-2 pages in RX path, with
no fallback to order-0 pages.

So I am not against this patch, but I can not really acknowledge it,
sorry.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rik van Riel May 29, 2013, 7:36 a.m. UTC | #4
On 05/27/2013 06:25 PM, Eric Dumazet wrote:
> On Mon, 2013-05-27 at 13:39 -0400, Rik van Riel wrote:


>> Yes please. Getting memory management bug reports for
>> dropped network packets got old years ago.  Lets get
>> rid of those messages.
>
> I am only wondering why this path has anything needing special
> attention, over thousands of kmalloc() like call sites in the kernel.

There are a few special things about the network code:

1) network packets can arrive extremely fast, in
    large batches
2) the network code cannot wait for the VM to free
    memory (GFP_ATOMIC)

Other allocations tend to be done less at a time, and/or
allow the VM to free up memory before proceeding.

> If mm allocation warnings are useless, just make __GFP_NOWARN the
> default, and save us thousand of patches (adding the __GFP_NOWARN
> everywhere)
>
> Truth is : some network drivers don't deal very well with allocation
> errors. mlx4 for example absolutely wants order-2 pages in RX path, with
> no fallback to order-0 pages.

Network protocols and network applications tend to deal
with packet loss by retransmitting data, though.

Also, once all the data from one of those order-2 page
buffers has been delivered or forwarded, that buffer
becomes available to subsequent network packets.

Other allocations tend not to free & reuse their
memory as quickly as the network stack.

> So I am not against this patch, but I can not really acknowledge it,
> sorry.
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 02139d6..84aa870 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -236,7 +236,7 @@  struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 		? skbuff_fclone_cache : skbuff_head_cache;
 
 	if (sk_memalloc_socks() && (flags & SKB_ALLOC_RX))
-		gfp_mask |= (__GFP_MEMALLOC|__GFP_NOWARN);
+		gfp_mask |= __GFP_MEMALLOC | __GFP_NOWARN;
 
 	/* Get the HEAD */
 	skb = kmem_cache_alloc_node(cache, gfp_mask & ~__GFP_DMA, node);