[net] tcp: refine memory limit test in tcp_fragment()
diff mbox series

Message ID 20190621130955.147974-1-edumazet@google.com
State Accepted
Delegated to: David Miller
Headers show
Series
  • [net] tcp: refine memory limit test in tcp_fragment()
Related show

Commit Message

Eric Dumazet June 21, 2019, 1:09 p.m. UTC
tcp_fragment() might be called for skbs in the write queue.

Memory limits might have been exceeded because tcp_sendmsg() only
checks limits at full skb (64KB) boundaries.

Therefore, we need to make sure tcp_fragment() wont punish applications
that might have setup very low SO_SNDBUF values.

Fixes: f070ef2ac667 ("tcp: tcp_fragment() should apply sane memory limits")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Christoph Paasch <cpaasch@apple.com>
---
 net/ipv4/tcp_output.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Christoph Paasch June 21, 2019, 2:50 p.m. UTC | #1
> On Jun 21, 2019, at 6:11 AM, Eric Dumazet <edumazet@google.com> wrote:
> 
> ´╗┐tcp_fragment() might be called for skbs in the write queue.
> 
> Memory limits might have been exceeded because tcp_sendmsg() only
> checks limits at full skb (64KB) boundaries.
> 
> Therefore, we need to make sure tcp_fragment() wont punish applications
> that might have setup very low SO_SNDBUF values.
> 
> Fixes: f070ef2ac667 ("tcp: tcp_fragment() should apply sane memory limits")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Christoph Paasch <cpaasch@apple.com>
> ---
> net/ipv4/tcp_output.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)

Tested-by: Christoph Paasch <cpaasch@apple.com>


Thanks!


> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 00c01a01b547ec67c971dc25a74c9258563cf871..0ebc33d1c9e5099d163a234930e213ee35e9fbd1 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1296,7 +1296,8 @@ int tcp_fragment(struct sock *sk, enum tcp_queue tcp_queue,
>    if (nsize < 0)
>        nsize = 0;
> 
> -    if (unlikely((sk->sk_wmem_queued >> 1) > sk->sk_sndbuf)) {
> +    if (unlikely((sk->sk_wmem_queued >> 1) > sk->sk_sndbuf &&
> +             tcp_queue != TCP_FRAG_IN_WRITE_QUEUE)) {
>        NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPWQUEUETOOBIG);
>        return -ENOMEM;
>    }
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
>
David Miller June 22, 2019, 1 a.m. UTC | #2
From: Eric Dumazet <edumazet@google.com>
Date: Fri, 21 Jun 2019 06:09:55 -0700

> tcp_fragment() might be called for skbs in the write queue.
> 
> Memory limits might have been exceeded because tcp_sendmsg() only
> checks limits at full skb (64KB) boundaries.
> 
> Therefore, we need to make sure tcp_fragment() wont punish applications
> that might have setup very low SO_SNDBUF values.
> 
> Fixes: f070ef2ac667 ("tcp: tcp_fragment() should apply sane memory limits")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Christoph Paasch <cpaasch@apple.com>

Applied and queued up for -stable.
Tony Lu July 3, 2019, 3:27 a.m. UTC | #3
Hello Eric,

	We have applied that commit e358f4af19db ("tcp: tcp_fragment() should apply sane memory limits")
	as a hotpatch in production environment. We found that it will make
	tcp long connection reset during sending out packet when applying
	that commit. 
	
	Our applications which in A/B test have suffered that
	and made them retransmit large data, and then caused retransmission
	storm and lower the performance and increase RT.

	Therefore we discontinued to apply this hotpatch in A/B test.

	After invesgation, we found this patch already fix this issue in
	stable. Before applying this patch, we have some questions:

	1. This commit in stable hard coded a magic number 0x20000. I am
	wondering this value and if there any better solution.
	2. Is there any known or unknown side effect? If any, we could test
	it in some suspicious scenarios before testing in prod env.

	Thanks.

Cheers,
Tony Lu

On Fri, Jun 21, 2019 at 06:09:55AM -0700, Eric Dumazet wrote:
> tcp_fragment() might be called for skbs in the write queue.
> 
> Memory limits might have been exceeded because tcp_sendmsg() only
> checks limits at full skb (64KB) boundaries.
> 
> Therefore, we need to make sure tcp_fragment() wont punish applications
> that might have setup very low SO_SNDBUF values.
> 
> Fixes: f070ef2ac667 ("tcp: tcp_fragment() should apply sane memory limits")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Christoph Paasch <cpaasch@apple.com>
> ---
>  net/ipv4/tcp_output.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 00c01a01b547ec67c971dc25a74c9258563cf871..0ebc33d1c9e5099d163a234930e213ee35e9fbd1 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1296,7 +1296,8 @@ int tcp_fragment(struct sock *sk, enum tcp_queue tcp_queue,
>  	if (nsize < 0)
>  		nsize = 0;
>  
> -	if (unlikely((sk->sk_wmem_queued >> 1) > sk->sk_sndbuf)) {
> +	if (unlikely((sk->sk_wmem_queued >> 1) > sk->sk_sndbuf &&
> +		     tcp_queue != TCP_FRAG_IN_WRITE_QUEUE)) {
>  		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPWQUEUETOOBIG);
>  		return -ENOMEM;
>  	}
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
Eric Dumazet July 3, 2019, 9:19 a.m. UTC | #4
On 7/2/19 8:27 PM, Tony Lu wrote:
> Hello Eric,
> 
> 	We have applied that commit e358f4af19db ("tcp: tcp_fragment() should apply sane memory limits")
> 	as a hotpatch in production environment. We found that it will make
> 	tcp long connection reset during sending out packet when applying
> 	that commit. 
> 	
> 	Our applications which in A/B test have suffered that
> 	and made them retransmit large data, and then caused retransmission
> 	storm and lower the performance and increase RT.
> 
> 	Therefore we discontinued to apply this hotpatch in A/B test.
> 
> 	After invesgation, we found this patch already fix this issue in
> 	stable. Before applying this patch, we have some questions:
> 

Which stable version are you referring to exactly ?

> 	1. This commit in stable hard coded a magic number 0x20000. I am
> 	wondering this value and if there any better solution.

0x20000 is two times 64KB, please read the changelog for the rationale.

> 	2. Is there any known or unknown side effect? If any, we could test
> 	it in some suspicious scenarios before testing in prod env.

No known side effect.

Honestly, applications setting small SO_SNDBUF values can not expect good TCP performance anyway.


> 
> 	Thanks.
> 
> Cheers,
> Tony Lu
> 
> On Fri, Jun 21, 2019 at 06:09:55AM -0700, Eric Dumazet wrote:
>> tcp_fragment() might be called for skbs in the write queue.
>>
>> Memory limits might have been exceeded because tcp_sendmsg() only
>> checks limits at full skb (64KB) boundaries.
>>
>> Therefore, we need to make sure tcp_fragment() wont punish applications
>> that might have setup very low SO_SNDBUF values.
>>
>> Fixes: f070ef2ac667 ("tcp: tcp_fragment() should apply sane memory limits")
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Reported-by: Christoph Paasch <cpaasch@apple.com>
>> ---
>>  net/ipv4/tcp_output.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
>> index 00c01a01b547ec67c971dc25a74c9258563cf871..0ebc33d1c9e5099d163a234930e213ee35e9fbd1 100644
>> --- a/net/ipv4/tcp_output.c
>> +++ b/net/ipv4/tcp_output.c
>> @@ -1296,7 +1296,8 @@ int tcp_fragment(struct sock *sk, enum tcp_queue tcp_queue,
>>  	if (nsize < 0)
>>  		nsize = 0;
>>  
>> -	if (unlikely((sk->sk_wmem_queued >> 1) > sk->sk_sndbuf)) {
>> +	if (unlikely((sk->sk_wmem_queued >> 1) > sk->sk_sndbuf &&
>> +		     tcp_queue != TCP_FRAG_IN_WRITE_QUEUE)) {
>>  		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPWQUEUETOOBIG);
>>  		return -ENOMEM;
>>  	}
>> -- 
>> 2.22.0.410.gd8fdbe21b5-goog

Patch
diff mbox series

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 00c01a01b547ec67c971dc25a74c9258563cf871..0ebc33d1c9e5099d163a234930e213ee35e9fbd1 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1296,7 +1296,8 @@  int tcp_fragment(struct sock *sk, enum tcp_queue tcp_queue,
 	if (nsize < 0)
 		nsize = 0;
 
-	if (unlikely((sk->sk_wmem_queued >> 1) > sk->sk_sndbuf)) {
+	if (unlikely((sk->sk_wmem_queued >> 1) > sk->sk_sndbuf &&
+		     tcp_queue != TCP_FRAG_IN_WRITE_QUEUE)) {
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPWQUEUETOOBIG);
 		return -ENOMEM;
 	}