[PATCH net] netlink: do not enter direct reclaim from netlink_dump()

[PATCH net] netlink: do not enter direct reclaim from netlink_dump()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Since linux-3.15, netlink_dump() can use up to 16384 bytes skb
allocations.

Due to struct skb_shared_info ~320 bytes overhead, we end up using
order-3 (on x86) page allocations, that might trigger direct reclaim and
add stress.

The intent was really to attempt a large allocation but immediately
fallback to a smaller one (order-1 on x86) in case of memory stress.

On recent kernels (linux-4.4), we can remove __GFP_DIRECT_RECLAIM to
meet the goal. Old kernels would need to remove __GFP_WAIT

While we are at it, since we do an order-3 allocation, allow to use
all the allocated bytes instead of 16384 to reduce syscalls during
large dumps.

iproute2 already uses 32KB recvmsg() buffer sizes.

Alexei provided an initial patch downsizing to SKB_WITH_OVERHEAD(16384)

Fixes: 9063e21fb026 ("netlink: autosize skb lengthes")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Alexei Starovoitov <ast@kernel.org>
Cc: Greg Thelen <gthelen@google.com>
---
Note: This will apply to net tree when it has synced with Linus tree.

 net/netlink/af_netlink.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 627f898c05b96552318a881ce995ccc3342e1576..62bea4591054820eb516ef016214ee23fe89b6e9 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1832,7 +1832,7 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
 	/* Record the max length of recvmsg() calls for future allocations */
 	nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
 	nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
-				     16384);
+				     SKB_WITH_OVERHEAD(32768));
 
 	copied = data_skb->len;
 	if (len < copied) {
@@ -2083,8 +2083,9 @@ static int netlink_dump(struct sock *sk)
 
 	if (alloc_min_size < nlk->max_recvmsg_len) {
 		alloc_size = nlk->max_recvmsg_len;
-		skb = alloc_skb(alloc_size, GFP_KERNEL |
-					    __GFP_NOWARN | __GFP_NORETRY);
+		skb = alloc_skb(alloc_size,
+				(GFP_KERNEL & ~__GFP_DIRECT_RECLAIM) |
+				__GFP_NOWARN | __GFP_NORETRY);
 	}
 	if (!skb) {
 		alloc_size = alloc_min_size;

Re: [PATCH net] netlink: do not enter direct reclaim from netlink_dump()

[Greg]

On Thu, 2016-10-06 at 04:13 +0900, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Since linux-3.15, netlink_dump() can use up to 16384 bytes skb
> allocations.
> 
> Due to struct skb_shared_info ~320 bytes overhead, we end up using
> order-3 (on x86) page allocations, that might trigger direct reclaim and
> add stress.
> 
> The intent was really to attempt a large allocation but immediately
> fallback to a smaller one (order-1 on x86) in case of memory stress.
> 
> On recent kernels (linux-4.4), we can remove __GFP_DIRECT_RECLAIM to
> meet the goal. Old kernels would need to remove __GFP_WAIT
> 
> While we are at it, since we do an order-3 allocation, allow to use
> all the allocated bytes instead of 16384 to reduce syscalls during
> large dumps.
> 
> iproute2 already uses 32KB recvmsg() buffer sizes.
> 
> Alexei provided an initial patch downsizing to SKB_WITH_OVERHEAD(16384)
> 
> Fixes: 9063e21fb026 ("netlink: autosize skb lengthes")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Alexei Starovoitov <ast@kernel.org>
> Cc: Greg Thelen <gthelen@google.com>
> ---
> Note: This will apply to net tree when it has synced with Linus tree.
> 
>  net/netlink/af_netlink.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 627f898c05b96552318a881ce995ccc3342e1576..62bea4591054820eb516ef016214ee23fe89b6e9 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -1832,7 +1832,7 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>  	/* Record the max length of recvmsg() calls for future allocations */
>  	nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
>  	nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
> -				     16384);
> +				     SKB_WITH_OVERHEAD(32768));
>  
>  	copied = data_skb->len;
>  	if (len < copied) {
> @@ -2083,8 +2083,9 @@ static int netlink_dump(struct sock *sk)
>  
>  	if (alloc_min_size < nlk->max_recvmsg_len) {
>  		alloc_size = nlk->max_recvmsg_len;
> -		skb = alloc_skb(alloc_size, GFP_KERNEL |
> -					    __GFP_NOWARN | __GFP_NORETRY);
> +		skb = alloc_skb(alloc_size,
> +				(GFP_KERNEL & ~__GFP_DIRECT_RECLAIM) |
> +				__GFP_NOWARN | __GFP_NORETRY);
>  	}
>  	if (!skb) {
>  		alloc_size = alloc_min_size;
> 
> 

This code has changed a lot since I first added it in 2011 but this
appears to be the right thing to do.  I guess the order of operations
for the bitwise '&' and the bitwise '~' are correct, I don't have my C
manual laying around.

Reviewed-by: Greg Rose <grose@lightfleet.com>

Re: [PATCH net] netlink: do not enter direct reclaim from netlink_dump()

[Alexei Starovoitov]

On Thu, Oct 06, 2016 at 04:13:18AM +0900, Eric Dumazet wrote:
> 
> While we are at it, since we do an order-3 allocation, allow to use
> all the allocated bytes instead of 16384 to reduce syscalls during
> large dumps.
> 
> iproute2 already uses 32KB recvmsg() buffer sizes.
....
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 627f898c05b96552318a881ce995ccc3342e1576..62bea4591054820eb516ef016214ee23fe89b6e9 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -1832,7 +1832,7 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>  	/* Record the max length of recvmsg() calls for future allocations */
>  	nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
>  	nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
> -				     16384);
> +				     SKB_WITH_OVERHEAD(32768));

sure, it won't stress it more than what it is today, but why increase it?
iproute2 increased the buffer form 16k to 32k due to 'msg_trunc' which
I think was due to this issue. If we go with SKB_WITH_OVERHEAD(16384)
we can go back to 16k in iproute2 as well.

Do we have any data to justify that buffer of 32k - skb_shared_info vs 16k
will meaninfully reduce the number of syscalls?
We're seeing direct reclaim get hammered due to order-3.
Not sure whether & ~__GFP_DIRECT_RECLAIM is going to be enough.
Currently we're testing with SKB_WITH_OVERHEAD(16384) and ~__GFP_DIRECT_RECLAIM.
It will take another week to make sure SKB_WITH_OVERHEAD(32768) is ok.
imo this optimization is done too soon.
I'd much more comfortable with SKB_WITH_OVERHEAD(16384) value here.

>  
>  	copied = data_skb->len;
>  	if (len < copied) {
> @@ -2083,8 +2083,9 @@ static int netlink_dump(struct sock *sk)
>  
>  	if (alloc_min_size < nlk->max_recvmsg_len) {
>  		alloc_size = nlk->max_recvmsg_len;
> -		skb = alloc_skb(alloc_size, GFP_KERNEL |
> -					    __GFP_NOWARN | __GFP_NORETRY);
> +		skb = alloc_skb(alloc_size,
> +				(GFP_KERNEL & ~__GFP_DIRECT_RECLAIM) |
> +				__GFP_NOWARN | __GFP_NORETRY);
>  	}
>  	if (!skb) {
>  		alloc_size = alloc_min_size;
> 
> 

Re: [PATCH net] netlink: do not enter direct reclaim from netlink_dump()

[Eric Dumazet]

On Wed, 2016-10-05 at 15:24 -0700, Alexei Starovoitov wrote:
> On Thu, Oct 06, 2016 at 04:13:18AM +0900, Eric Dumazet wrote:
> > 
> > While we are at it, since we do an order-3 allocation, allow to use
> > all the allocated bytes instead of 16384 to reduce syscalls during
> > large dumps.
> > 
> > iproute2 already uses 32KB recvmsg() buffer sizes.
> ....
> > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > index 627f898c05b96552318a881ce995ccc3342e1576..62bea4591054820eb516ef016214ee23fe89b6e9 100644
> > --- a/net/netlink/af_netlink.c
> > +++ b/net/netlink/af_netlink.c
> > @@ -1832,7 +1832,7 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> >  	/* Record the max length of recvmsg() calls for future allocations */
> >  	nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
> >  	nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
> > -				     16384);
> > +				     SKB_WITH_OVERHEAD(32768));
> 
> sure, it won't stress it more than what it is today, but why increase it?
> iproute2 increased the buffer form 16k to 32k due to 'msg_trunc' which
> I think was due to this issue. If we go with SKB_WITH_OVERHEAD(16384)
> we can go back to 16k in iproute2 as well.
> 
> Do we have any data to justify that buffer of 32k - skb_shared_info vs 16k
> will meaninfully reduce the number of syscalls?
> We're seeing direct reclaim get hammered due to order-3.
> Not sure whether & ~__GFP_DIRECT_RECLAIM is going to be enough.

It is. Really.

> Currently we're testing with SKB_WITH_OVERHEAD(16384) and ~__GFP_DIRECT_RECLAIM.
> It will take another week to make sure SKB_WITH_OVERHEAD(32768) is ok.
> imo this optimization is done too soon.
> I'd much more comfortable with SKB_WITH_OVERHEAD(16384) value here.

Well, we _are_ allocating order-3 pages already.

No need to switch to order-2 pages, when we have the proper fix.

Note that tcp_sendmsg() does this all the time, and nobody complained
after Shaohua Li fix (commit fb05e7a89f500cf "net: don't wait for
order-3 page allocation")

Why thousands of sockets could use order-3 pages, but constrain _one_
(rtnl serializations) iproute2 dump to use tiny blocs exactly ?

The rationale for order-3 is pretty clear : 

#define PAGE_ALLOC_COSTLY_ORDER 3

Really there is no point being cautious here.

Re: [PATCH net] netlink: do not enter direct reclaim from netlink_dump()

[Eric Dumazet]

On Wed, 2016-10-05 at 15:24 -0700, Alexei Starovoitov wrote:
> On Thu, Oct 06, 2016 at 04:13:18AM +0900, Eric Dumazet wrote:
> > 
> > While we are at it, since we do an order-3 allocation, allow to use
> > all the allocated bytes instead of 16384 to reduce syscalls during
> > large dumps.
> > 
> > iproute2 already uses 32KB recvmsg() buffer sizes.
> ....
> > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > index 627f898c05b96552318a881ce995ccc3342e1576..62bea4591054820eb516ef016214ee23fe89b6e9 100644
> > --- a/net/netlink/af_netlink.c
> > +++ b/net/netlink/af_netlink.c
> > @@ -1832,7 +1832,7 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> >  	/* Record the max length of recvmsg() calls for future allocations */
> >  	nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
> >  	nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
> > -				     16384);
> > +				     SKB_WITH_OVERHEAD(32768));
> 
> sure, it won't stress it more than what it is today, but why increase it?
> iproute2 increased the buffer form 16k to 32k due to 'msg_trunc' which
> I think was due to this issue. If we go with SKB_WITH_OVERHEAD(16384)
> we can go back to 16k in iproute2 as well.

Wow, if really iproute2 tool would have increased the buffer to work
around a bug in the kernel, we should be worried.

Hopefully the issue was fixed for good in the kernel ?

commit db65a3aaf29ecce2e34271d52e8d2336b97bd9fe
("netlink: Trim skb to alloc size to avoid MSG_TRUNC")

Re: [PATCH net] netlink: do not enter direct reclaim from netlink_dump()

[Alexei Starovoitov]

On Thu, Oct 06, 2016 at 08:35:21AM +0900, Eric Dumazet wrote:
> On Wed, 2016-10-05 at 15:24 -0700, Alexei Starovoitov wrote:
> > On Thu, Oct 06, 2016 at 04:13:18AM +0900, Eric Dumazet wrote:
> > > 
> > > While we are at it, since we do an order-3 allocation, allow to use
> > > all the allocated bytes instead of 16384 to reduce syscalls during
> > > large dumps.
> > > 
> > > iproute2 already uses 32KB recvmsg() buffer sizes.
> > ....
> > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > > index 627f898c05b96552318a881ce995ccc3342e1576..62bea4591054820eb516ef016214ee23fe89b6e9 100644
> > > --- a/net/netlink/af_netlink.c
> > > +++ b/net/netlink/af_netlink.c
> > > @@ -1832,7 +1832,7 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> > >  	/* Record the max length of recvmsg() calls for future allocations */
> > >  	nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
> > >  	nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
> > > -				     16384);
> > > +				     SKB_WITH_OVERHEAD(32768));
> > 
> > sure, it won't stress it more than what it is today, but why increase it?
> > iproute2 increased the buffer form 16k to 32k due to 'msg_trunc' which
> > I think was due to this issue. If we go with SKB_WITH_OVERHEAD(16384)
> > we can go back to 16k in iproute2 as well.
> 
> Wow, if really iproute2 tool would have increased the buffer to work
> around a bug in the kernel, we should be worried.
> 
> Hopefully the issue was fixed for good in the kernel ?
> 
> commit db65a3aaf29ecce2e34271d52e8d2336b97bd9fe
> ("netlink: Trim skb to alloc size to avoid MSG_TRUNC")

I would think so too, but
iproute2 'fix' 72b365e8e0fd ("libnetlink: Double the dump buffer size")
was on Mar 4, 2016
whereas above 'netlink: trim' fix is on Oct 15, 2015

Re: [PATCH net] netlink: do not enter direct reclaim from netlink_dump()

[Alexei Starovoitov]

On Thu, Oct 06, 2016 at 08:30:11AM +0900, Eric Dumazet wrote:
> On Wed, 2016-10-05 at 15:24 -0700, Alexei Starovoitov wrote:
> > On Thu, Oct 06, 2016 at 04:13:18AM +0900, Eric Dumazet wrote:
> > > 
> > > While we are at it, since we do an order-3 allocation, allow to use
> > > all the allocated bytes instead of 16384 to reduce syscalls during
> > > large dumps.
> > > 
> > > iproute2 already uses 32KB recvmsg() buffer sizes.
> > ....
> > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > > index 627f898c05b96552318a881ce995ccc3342e1576..62bea4591054820eb516ef016214ee23fe89b6e9 100644
> > > --- a/net/netlink/af_netlink.c
> > > +++ b/net/netlink/af_netlink.c
> > > @@ -1832,7 +1832,7 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> > >  	/* Record the max length of recvmsg() calls for future allocations */
> > >  	nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
> > >  	nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
> > > -				     16384);
> > > +				     SKB_WITH_OVERHEAD(32768));
> > 
> > sure, it won't stress it more than what it is today, but why increase it?
> > iproute2 increased the buffer form 16k to 32k due to 'msg_trunc' which
> > I think was due to this issue. If we go with SKB_WITH_OVERHEAD(16384)
> > we can go back to 16k in iproute2 as well.
> > 
> > Do we have any data to justify that buffer of 32k - skb_shared_info vs 16k
> > will meaninfully reduce the number of syscalls?
> > We're seeing direct reclaim get hammered due to order-3.
> > Not sure whether & ~__GFP_DIRECT_RECLAIM is going to be enough.
> 
> It is. Really.
> 
> > Currently we're testing with SKB_WITH_OVERHEAD(16384) and ~__GFP_DIRECT_RECLAIM.
> > It will take another week to make sure SKB_WITH_OVERHEAD(32768) is ok.
> > imo this optimization is done too soon.
> > I'd much more comfortable with SKB_WITH_OVERHEAD(16384) value here.
> 
> Well, we _are_ allocating order-3 pages already.
> 
> No need to switch to order-2 pages, when we have the proper fix.
> 
> Note that tcp_sendmsg() does this all the time, and nobody complained
> after Shaohua Li fix (commit fb05e7a89f500cf "net: don't wait for
> order-3 page allocation")
> 
> Why thousands of sockets could use order-3 pages, but constrain _one_
> (rtnl serializations) iproute2 dump to use tiny blocs exactly ?

Good point. Large tcp_sendmsg() should be stressing mm
with order-3 more than netlink polling once a second
that some application do with 'ss' or 'tc -s show'

> Really there is no point being cautious here.

I guess I'm being too paranoid. If we discover issues
with SKB_WITH_OVERHEAD(32768), we can adjust it later, so
Acked-by: Alexei Starovoitov <ast@kernel.org>

Re: [PATCH net] netlink: do not enter direct reclaim from netlink_dump()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 06 Oct 2016 04:13:18 +0900

> From: Eric Dumazet <edumazet@google.com>
> 
> Since linux-3.15, netlink_dump() can use up to 16384 bytes skb
> allocations.
> 
> Due to struct skb_shared_info ~320 bytes overhead, we end up using
> order-3 (on x86) page allocations, that might trigger direct reclaim and
> add stress.
> 
> The intent was really to attempt a large allocation but immediately
> fallback to a smaller one (order-1 on x86) in case of memory stress.
> 
> On recent kernels (linux-4.4), we can remove __GFP_DIRECT_RECLAIM to
> meet the goal. Old kernels would need to remove __GFP_WAIT
> 
> While we are at it, since we do an order-3 allocation, allow to use
> all the allocated bytes instead of 16384 to reduce syscalls during
> large dumps.
> 
> iproute2 already uses 32KB recvmsg() buffer sizes.
> 
> Alexei provided an initial patch downsizing to SKB_WITH_OVERHEAD(16384)
> 
> Fixes: 9063e21fb026 ("netlink: autosize skb lengthes")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Alexei Starovoitov <ast@kernel.org>
> Cc: Greg Thelen <gthelen@google.com>
> ---
> Note: This will apply to net tree when it has synced with Linus tree.

Applied.