[PATCH net-next] tcp: suppress too verbose messages in tcp_send_ack()

[PATCH net-next] tcp: suppress too verbose messages in tcp_send_ack()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

If tcp_send_ack() can not allocate skb, we properly handle this
and setup a timer to try later.

Use __GFP_NOWARN to avoid polluting syslog in the case host is
under memory pressure, so that pertinent messages are not lost under
a flood of useless information.

sk_gfp_atomic() can use its gfp_mask argument (all callers currently
were using GFP_ATOMIC before this patch)

Note that when tcp_transmit_skb() is called with clone_it set to false,
we do not attempt memory allocations, so can pass a 0 gfp_mask, which
most compilers can emit faster than a non zero value.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/sock.h    |    2 +-
 net/ipv4/tcp_output.c |   12 +++++++-----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 7f89e4ba18d1..ead514332ae8 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -776,7 +776,7 @@ static inline int sk_memalloc_socks(void)
 
 static inline gfp_t sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask)
 {
-	return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC);
+	return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC);
 }
 
 static inline void sk_acceptq_removed(struct sock *sk)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index cb7ca569052c..0a1d4f6ab52f 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3352,8 +3352,9 @@ void tcp_send_ack(struct sock *sk)
 	 * tcp_transmit_skb() will set the ownership to this
 	 * sock.
 	 */
-	buff = alloc_skb(MAX_TCP_HEADER, sk_gfp_atomic(sk, GFP_ATOMIC));
-	if (!buff) {
+	buff = alloc_skb(MAX_TCP_HEADER,
+			 sk_gfp_atomic(sk, GFP_ATOMIC | __GFP_NOWARN));
+	if (unlikely(!buff)) {
 		inet_csk_schedule_ack(sk);
 		inet_csk(sk)->icsk_ack.ato = TCP_ATO_MIN;
 		inet_csk_reset_xmit_timer(sk, ICSK_TIME_DACK,
@@ -3375,7 +3376,7 @@ void tcp_send_ack(struct sock *sk)
 
 	/* Send it off, this clears delayed acks for us. */
 	skb_mstamp_get(&buff->skb_mstamp);
-	tcp_transmit_skb(sk, buff, 0, sk_gfp_atomic(sk, GFP_ATOMIC));
+	tcp_transmit_skb(sk, buff, 0, (__force gfp_t)0);
 }
 EXPORT_SYMBOL_GPL(tcp_send_ack);
 
@@ -3396,7 +3397,8 @@ static int tcp_xmit_probe_skb(struct sock *sk, int urgent, int mib)
 	struct sk_buff *skb;
 
 	/* We don't queue it, tcp_transmit_skb() sets ownership. */
-	skb = alloc_skb(MAX_TCP_HEADER, sk_gfp_atomic(sk, GFP_ATOMIC));
+	skb = alloc_skb(MAX_TCP_HEADER,
+			sk_gfp_atomic(sk, GFP_ATOMIC | __GFP_NOWARN));
 	if (!skb)
 		return -1;
 
@@ -3409,7 +3411,7 @@ static int tcp_xmit_probe_skb(struct sock *sk, int urgent, int mib)
 	tcp_init_nondata_skb(skb, tp->snd_una - !urgent, TCPHDR_ACK);
 	skb_mstamp_get(&skb->skb_mstamp);
 	NET_INC_STATS(sock_net(sk), mib);
-	return tcp_transmit_skb(sk, skb, 0, GFP_ATOMIC);
+	return tcp_transmit_skb(sk, skb, 0, (__force gfp_t)0);
 }
 
 void tcp_send_window_probe(struct sock *sk)

Re: [PATCH net-next] tcp: suppress too verbose messages in tcp_send_ack()

[Aaron Conole]

Eric Dumazet <eric.dumazet@gmail.com> writes:
> From: Eric Dumazet <edumazet@google.com>
>
> If tcp_send_ack() can not allocate skb, we properly handle this
> and setup a timer to try later.
>
> Use __GFP_NOWARN to avoid polluting syslog in the case host is
> under memory pressure, so that pertinent messages are not lost under
> a flood of useless information.
>
> sk_gfp_atomic() can use its gfp_mask argument (all callers currently
> were using GFP_ATOMIC before this patch)
>
> Note that when tcp_transmit_skb() is called with clone_it set to false,
> we do not attempt memory allocations, so can pass a 0 gfp_mask, which
> most compilers can emit faster than a non zero value.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  include/net/sock.h    |    2 +-
>  net/ipv4/tcp_output.c |   12 +++++++-----
>  2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 7f89e4ba18d1..ead514332ae8 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -776,7 +776,7 @@ static inline int sk_memalloc_socks(void)
>  
>  static inline gfp_t sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask)
>  {
> -	return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC);
> +	return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC);
>  }
>  

Sorry if I'm missing something obvious here, but with a name like
sk_gfp_atomic, would it make sense to keep the GFP_ATOMIC mask as well?
Otherwise, what is the _atomic is saying?

Thanks,
Aaron

Re: [PATCH net-next] tcp: suppress too verbose messages in tcp_send_ack()

[Eric Dumazet]

On Wed, 2015-11-25 at 17:08 -0500, Aaron Conole wrote:

> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index 7f89e4ba18d1..ead514332ae8 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -776,7 +776,7 @@ static inline int sk_memalloc_socks(void)
> >  
> >  static inline gfp_t sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask)
> >  {
> > -	return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC);
> > +	return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC);
> >  }
> >  
> 
> Sorry if I'm missing something obvious here, but with a name like
> sk_gfp_atomic, would it make sense to keep the GFP_ATOMIC mask as well?
> Otherwise, what is the _atomic is saying?

Not sure what you suggest.

Are you suggesting I remove GFP_ATOMIC from all callers ?

I am fine with this, but looks more invasive, and who knows, maybe one
caller might want to not use GFP_ATOMIC one day (like : do not attempt
to use reserves)

This sk_gfp_atomic() helper has a misleading name, since all it wanted
was to conditionally OR a caller provided flag (mostly GFP_ATOMIC one)
with __GFP_MEMALLOC for some special sockets.

Should have been sk_gfp_or_memalloc() or something...

Re: [PATCH net-next] tcp: suppress too verbose messages in tcp_send_ack()

[Eric Dumazet]

On Wed, 2015-11-25 at 14:32 -0800, Eric Dumazet wrote:
> On Wed, 2015-11-25 at 17:08 -0500, Aaron Conole wrote:
> 
> > > diff --git a/include/net/sock.h b/include/net/sock.h
> > > index 7f89e4ba18d1..ead514332ae8 100644
> > > --- a/include/net/sock.h
> > > +++ b/include/net/sock.h
> > > @@ -776,7 +776,7 @@ static inline int sk_memalloc_socks(void)
> > >  
> > >  static inline gfp_t sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask)
> > >  {
> > > -	return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC);
> > > +	return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC);
> > >  }
> > >  
> > 
> > Sorry if I'm missing something obvious here, but with a name like
> > sk_gfp_atomic, would it make sense to keep the GFP_ATOMIC mask as well?
> > Otherwise, what is the _atomic is saying?
> 
> Not sure what you suggest.
> 
> Are you suggesting I remove GFP_ATOMIC from all callers ?
> 
> I am fine with this, but looks more invasive, and who knows, maybe one
> caller might want to not use GFP_ATOMIC one day (like : do not attempt
> to use reserves)
> 
> This sk_gfp_atomic() helper has a misleading name, since all it wanted
> was to conditionally OR a caller provided flag (mostly GFP_ATOMIC one)
> with __GFP_MEMALLOC for some special sockets.
> 
> Should have been sk_gfp_or_memalloc() or something...
> 

BTW original commit changelog was clear and matches my expectations :

commit 99a1dec70d5acbd8c6b3928cdebb4a2d1da676c8
Author: Mel Gorman <mgorman@suse.de>
Date:   Tue Jul 31 16:44:14 2012 -0700

    net: introduce sk_gfp_atomic() to allow addition of GFP flags depending on the individual socket
    
    Introduce sk_gfp_atomic(), this function allows to inject sock specific
    flags to each sock related allocation.  It is only used on allocation
    paths that may be required for writing pages back to network storage.

Re: [PATCH net-next] tcp: suppress too verbose messages in tcp_send_ack()

[Aaron Conole]

Eric Dumazet <eric.dumazet@gmail.com> writes:
> On Wed, 2015-11-25 at 14:32 -0800, Eric Dumazet wrote:
>> On Wed, 2015-11-25 at 17:08 -0500, Aaron Conole wrote:
>> 
>> > > diff --git a/include/net/sock.h b/include/net/sock.h
>> > > index 7f89e4ba18d1..ead514332ae8 100644
>> > > --- a/include/net/sock.h
>> > > +++ b/include/net/sock.h
>> > > @@ -776,7 +776,7 @@ static inline int sk_memalloc_socks(void)
>> > >  
>> > >  static inline gfp_t sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask)
>> > >  {
>> > > -	return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC);
>> > > +	return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC);
>> > >  }
>> > >  
>> > 
>> > Sorry if I'm missing something obvious here, but with a name like
>> > sk_gfp_atomic, would it make sense to keep the GFP_ATOMIC mask as well?
>> > Otherwise, what is the _atomic is saying?
>> 
>> Not sure what you suggest.
>> 
>> Are you suggesting I remove GFP_ATOMIC from all callers ?

That's an option, and one that looks pretty clean.

>> I am fine with this, but looks more invasive, and who knows, maybe one
>> caller might want to not use GFP_ATOMIC one day (like : do not attempt
>> to use reserves)

Probably that would call for a different more primitive version of this
API (sk_gfp_or_memalloc() as you suggest below). Then this could be
written in terms of that

static inline sk_gfp_or_memalloc(const struct sock *sk, gfp_t gfp_mask)
{
	return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC);
}

static inline sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask)
{
	return sk_gfp_or_memalloc(sk, gfp_mask | GFP_ATOMIC);
}

Not sure if it's "too much API".

>> This sk_gfp_atomic() helper has a misleading name, since all it wanted
>> was to conditionally OR a caller provided flag (mostly GFP_ATOMIC one)
>> with __GFP_MEMALLOC for some special sockets.
>> 
>> Should have been sk_gfp_or_memalloc() or something...
>> 
>
> BTW original commit changelog was clear and matches my expectations :
>
> commit 99a1dec70d5acbd8c6b3928cdebb4a2d1da676c8
> Author: Mel Gorman <mgorman@suse.de>
> Date:   Tue Jul 31 16:44:14 2012 -0700
>
>     net: introduce sk_gfp_atomic() to allow addition of GFP flags
> depending on the individual socket
>     
>     Introduce sk_gfp_atomic(), this function allows to inject sock specific
>     flags to each sock related allocation.  It is only used on allocation
>     paths that may be required for writing pages back to network storage.

Cool. If you think my suggestion is too much for this set, that's
fine. I understand not wanting to be too intrusive.

Thanks,
Aaron

Re: [PATCH net-next] tcp: suppress too verbose messages in tcp_send_ack()

[Eric Dumazet]

On Wed, 2015-11-25 at 22:17 -0500, Aaron Conole wrote:

> Probably that would call for a different more primitive version of this
> API (sk_gfp_or_memalloc() as you suggest below). Then this could be
> written in terms of that
> 
> static inline sk_gfp_or_memalloc(const struct sock *sk, gfp_t gfp_mask)
> {
> 	return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC);
> }
> 
> static inline sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask)
> {
> 	return sk_gfp_or_memalloc(sk, gfp_mask | GFP_ATOMIC);
> }
> 
> Not sure if it's "too much API".

Well, this looks like it, not sure how this is going to make code
clearer.

The only thing we bring from sk is the __GFP_MEMALLOC thing, so a single
function seems enough ?

I honestly do not care that much about function names, I mostly look at
actual implementation. And current implementation ignores the gfp_t
gfp_mask argument, for no real good reason.

Re: [PATCH net-next] tcp: suppress too verbose messages in tcp_send_ack()

[Aaron Conole]

Eric Dumazet <eric.dumazet@gmail.com> writes:
> On Wed, 2015-11-25 at 22:17 -0500, Aaron Conole wrote:
>
>> Probably that would call for a different more primitive version of this
>> API (sk_gfp_or_memalloc() as you suggest below). Then this could be
>> written in terms of that
>> 
>> static inline sk_gfp_or_memalloc(const struct sock *sk, gfp_t gfp_mask)
>> {
>> 	return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC);
>> }
>> 
>> static inline sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask)
>> {
>> 	return sk_gfp_or_memalloc(sk, gfp_mask | GFP_ATOMIC);
>> }
>> 
>> Not sure if it's "too much API".
>
> Well, this looks like it, not sure how this is going to make code
> clearer.
>
> The only thing we bring from sk is the __GFP_MEMALLOC thing, so a single
> function seems enough ?

Okay. Just thought that a 'gfp' function ending in _atomic that doesn't
actually set GFP_ATOMIC might now confuse, but if you think it's no big
deal, hey no skin off my back :)

> I honestly do not care that much about function names, I mostly look at
> actual implementation. And current implementation ignores the gfp_t
> gfp_mask argument, for no real good reason.

I agree with this.

Re: [PATCH net-next] tcp: suppress too verbose messages in tcp_send_ack()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 25 Nov 2015 13:50:50 -0800

> diff --git a/include/net/sock.h b/include/net/sock.h
> index 7f89e4ba18d1..ead514332ae8 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -776,7 +776,7 @@ static inline int sk_memalloc_socks(void)
>  
>  static inline gfp_t sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask)
>  {
> -	return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC);
> +	return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC);
>  }
>  
>  static inline void sk_acceptq_removed(struct sock *sk)

Eric, please rename this to "sk_gfp_mask()" or "sk_gfp_flags()" or
something like that since it doesn't unconditionally use GFP_ATOMIC
any more.

Otherwise I'm %100 fine with this change.

Thank you.

[PATCH v2 net-next] tcp: suppress too verbose messages in tcp_send_ack()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

If tcp_send_ack() can not allocate skb, we properly handle this
and setup a timer to try later.

Use __GFP_NOWARN to avoid polluting syslog in the case host is
under memory pressure, so that pertinent messages are not lost under
a flood of useless information.

sk_gfp_atomic() can use its gfp_mask argument (all callers currently
were using GFP_ATOMIC before this patch)

We rename sk_gfp_atomic() to sk_gfp_mask() to clearly express this
function now takes into account its second argument (gfp_mask)

Note that when tcp_transmit_skb() is called with clone_it set to false,
we do not attempt memory allocations, so can pass a 0 gfp_mask, which
most compilers can emit faster than a non zero or constant value.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
v2: rename sk_gfp_atomic() to sk_gfp_mask()

 include/net/sock.h    |    4 ++--
 net/ipv4/tcp_output.c |   14 ++++++++------
 net/ipv6/tcp_ipv6.c   |    6 +++---
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 7f89e4ba18d1..89073bda77df 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -774,9 +774,9 @@ static inline int sk_memalloc_socks(void)
 
 #endif
 
-static inline gfp_t sk_gfp_atomic(const struct sock *sk, gfp_t gfp_mask)
+static inline gfp_t sk_gfp_mask(const struct sock *sk, gfp_t gfp_mask)
 {
-	return GFP_ATOMIC | (sk->sk_allocation & __GFP_MEMALLOC);
+	return gfp_mask | (sk->sk_allocation & __GFP_MEMALLOC);
 }
 
 static inline void sk_acceptq_removed(struct sock *sk)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index cb7ca569052c..a800cee88035 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2296,7 +2296,7 @@ void __tcp_push_pending_frames(struct sock *sk, unsigned int cur_mss,
 		return;
 
 	if (tcp_write_xmit(sk, cur_mss, nonagle, 0,
-			   sk_gfp_atomic(sk, GFP_ATOMIC)))
+			   sk_gfp_mask(sk, GFP_ATOMIC)))
 		tcp_check_probe_timer(sk);
 }
 
@@ -3352,8 +3352,9 @@ void tcp_send_ack(struct sock *sk)
 	 * tcp_transmit_skb() will set the ownership to this
 	 * sock.
 	 */
-	buff = alloc_skb(MAX_TCP_HEADER, sk_gfp_atomic(sk, GFP_ATOMIC));
-	if (!buff) {
+	buff = alloc_skb(MAX_TCP_HEADER,
+			 sk_gfp_mask(sk, GFP_ATOMIC | __GFP_NOWARN));
+	if (unlikely(!buff)) {
 		inet_csk_schedule_ack(sk);
 		inet_csk(sk)->icsk_ack.ato = TCP_ATO_MIN;
 		inet_csk_reset_xmit_timer(sk, ICSK_TIME_DACK,
@@ -3375,7 +3376,7 @@ void tcp_send_ack(struct sock *sk)
 
 	/* Send it off, this clears delayed acks for us. */
 	skb_mstamp_get(&buff->skb_mstamp);
-	tcp_transmit_skb(sk, buff, 0, sk_gfp_atomic(sk, GFP_ATOMIC));
+	tcp_transmit_skb(sk, buff, 0, (__force gfp_t)0);
 }
 EXPORT_SYMBOL_GPL(tcp_send_ack);
 
@@ -3396,7 +3397,8 @@ static int tcp_xmit_probe_skb(struct sock *sk, int urgent, int mib)
 	struct sk_buff *skb;
 
 	/* We don't queue it, tcp_transmit_skb() sets ownership. */
-	skb = alloc_skb(MAX_TCP_HEADER, sk_gfp_atomic(sk, GFP_ATOMIC));
+	skb = alloc_skb(MAX_TCP_HEADER,
+			sk_gfp_mask(sk, GFP_ATOMIC | __GFP_NOWARN));
 	if (!skb)
 		return -1;
 
@@ -3409,7 +3411,7 @@ static int tcp_xmit_probe_skb(struct sock *sk, int urgent, int mib)
 	tcp_init_nondata_skb(skb, tp->snd_una - !urgent, TCPHDR_ACK);
 	skb_mstamp_get(&skb->skb_mstamp);
 	NET_INC_STATS(sock_net(sk), mib);
-	return tcp_transmit_skb(sk, skb, 0, GFP_ATOMIC);
+	return tcp_transmit_skb(sk, skb, 0, (__force gfp_t)0);
 }
 
 void tcp_send_window_probe(struct sock *sk)
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index c5429a636f1a..41bcd59a2ac7 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1130,7 +1130,7 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
 		 */
 		tcp_md5_do_add(newsk, (union tcp_md5_addr *)&newsk->sk_v6_daddr,
 			       AF_INET6, key->key, key->keylen,
-			       sk_gfp_atomic(sk, GFP_ATOMIC));
+			       sk_gfp_mask(sk, GFP_ATOMIC));
 	}
 #endif
 
@@ -1146,7 +1146,7 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
 		/* Clone pktoptions received with SYN, if we own the req */
 		if (ireq->pktopts) {
 			newnp->pktoptions = skb_clone(ireq->pktopts,
-						      sk_gfp_atomic(sk, GFP_ATOMIC));
+						      sk_gfp_mask(sk, GFP_ATOMIC));
 			consume_skb(ireq->pktopts);
 			ireq->pktopts = NULL;
 			if (newnp->pktoptions)
@@ -1212,7 +1212,7 @@ static int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 					       --ANK (980728)
 	 */
 	if (np->rxopt.all)
-		opt_skb = skb_clone(skb, sk_gfp_atomic(sk, GFP_ATOMIC));
+		opt_skb = skb_clone(skb, sk_gfp_mask(sk, GFP_ATOMIC));
 
 	if (sk->sk_state == TCP_ESTABLISHED) { /* Fast path */
 		struct dst_entry *dst = sk->sk_rx_dst;

Re: [PATCH v2 net-next] tcp: suppress too verbose messages in tcp_send_ack()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 30 Nov 2015 08:57:28 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> If tcp_send_ack() can not allocate skb, we properly handle this
> and setup a timer to try later.
> 
> Use __GFP_NOWARN to avoid polluting syslog in the case host is
> under memory pressure, so that pertinent messages are not lost under
> a flood of useless information.
> 
> sk_gfp_atomic() can use its gfp_mask argument (all callers currently
> were using GFP_ATOMIC before this patch)
> 
> We rename sk_gfp_atomic() to sk_gfp_mask() to clearly express this
> function now takes into account its second argument (gfp_mask)
> 
> Note that when tcp_transmit_skb() is called with clone_it set to false,
> we do not attempt memory allocations, so can pass a 0 gfp_mask, which
> most compilers can emit faster than a non zero or constant value.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> v2: rename sk_gfp_atomic() to sk_gfp_mask()

Applied, thanks Eric.