net: Unbreak resetting default values for tcp_wmem/udp_wmem_min
Subsystems (2)
PATCH 1/1 net: Unbreak resetting default values for tcp_wmem/udp_wmem_min
Commit 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values written to them are not less than SOCK_MIN_{RCV,SND}BUF.
This change is fine for tcp_rmem and udp_rmem_min, since SOCK_MIN_RCVBUF is equal to equal to TCP_SKB_MIN_TRUESIZE. But it breaks tcp_wmem and udp_wmem_min for previously valid values because SOCK_MIN_SNDBUF is (2 * TCP_SKB_MIN_TRUESIZE), which ends up being greater than 4KB.
Thus, 4096 is no longer accepted as a valid value, despite still being the default for udp_wmem_min, and for 'min' in tcp_wmem. A huge number of sysctl configurations at FB use 4096 as 'min', so this change breaks all of them.
This patch changes the sysctls to simply enforce that the value written is greater than or equal to the default value of SK_MEM_QUANTUM.
--- net/ipv4/sysctl_net_ipv4.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
net/ipv4/sysctl_net_ipv4.c +5 -6
@@ -41,8 +41,7 @@ static int tcp_syn_retries_min = 1;static int tcp_syn_retries_max =MAX_TCP_SYNCNT ;static int ip_ping_group_range_min[] = {0 ,0 };static int ip_ping_group_range_max[] = {GID_T_MAX ,GID_T_MAX }; -static int min_sndbuf =SOCK_MIN_SNDBUF ; -static int min_rcvbuf =SOCK_MIN_RCVBUF ; +static int min_buf =SK_MEM_QUANTUM ;/* Update system visible IP port range */ static void set_local_port_range (struct net *net,int range[2 ])
@@ -530,7 +529,7 @@ static struct ctl_table ipv4_table[] = { .maxlen = sizeof(sysctl_tcp_wmem), .mode = 0644, .proc_handler = proc_dointvec_minmax, - .extra1 = &min_sndbuf, + .extra1 = &min_buf, }, { .procname = "tcp_notsent_lowat",
@@ -545,7 +544,7 @@ static struct ctl_table ipv4_table[] = { .maxlen = sizeof(sysctl_tcp_rmem), .mode = 0644, .proc_handler = proc_dointvec_minmax, - .extra1 = &min_rcvbuf, + .extra1 = &min_buf, }, { .procname = "tcp_app_win",
@@ -758,7 +757,7 @@ static struct ctl_table ipv4_table[] = { .maxlen = sizeof(sysctl_udp_rmem_min), .mode = 0644, .proc_handler = proc_dointvec_minmax, - .extra1 = &min_rcvbuf, + .extra1 = &min_buf, }, { .procname = "udp_wmem_min",
@@ -766,7 +765,7 @@ static struct ctl_table ipv4_table[] = { .maxlen = sizeof(sysctl_udp_wmem_min), .mode = 0644, .proc_handler = proc_dointvec_minmax, - .extra1 = &min_sndbuf, + .extra1 = &min_buf, }, { } };
Signature · 1 line
1.8.1
David Miller
maintainerFrom: Calvin Owens <[email protected]>
Date: Wed, 5 Aug 2015 13:26:54 -0700
Quoted 14 lines
Commit
8133534c760d4083("net: limit tcp/udp rmem/wmem to SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values written to them are not less than SOCK_MIN_{RCV,SND}BUF.This change is fine for
tcp_rmemandudp_rmem_min, sinceSOCK_MIN_RCVBUFis equal to equal toTCP_SKB_MIN_TRUESIZE. But it breakstcp_wmemandudp_wmem_minfor previously valid values becauseSOCK_MIN_SNDBUFis (2 *TCP_SKB_MIN_TRUESIZE), which ends up being greater than 4KB.Thus, 4096 is no longer accepted as a valid value, despite still being the default for
udp_wmem_min, and for 'min' intcp_wmem. A huge number of sysctl configurations at FB use 4096 as 'min', so this change breaks all of them.
Quoted 5 lines
This patch changes the sysctls to simply enforce that the value written is greater than or equal to the default value of
SK_MEM_QUANTUM.Fixes 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")Signed-off-by Calvin Owens <[email protected]>
I think increasing the default makes more sense.
If we don't allow applications to set 4K, the kernel shouldn't start with that value either.
Quoted 20 lines · David Miller
From: Calvin Owens <[email protected]>
Date: Wed, 5 Aug 2015 13:26:54 -0700Quoted 17 lines
Commit
8133534c760d4083("net: limit tcp/udp rmem/wmem to SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values written to them are not less than SOCK_MIN_{RCV,SND}BUF.This change is fine for
tcp_rmemandudp_rmem_min, sinceSOCK_MIN_RCVBUFis equal to equal toTCP_SKB_MIN_TRUESIZE. But it breakstcp_wmemandudp_wmem_minfor previously valid values becauseSOCK_MIN_SNDBUFis (2 *TCP_SKB_MIN_TRUESIZE), which ends up being greater than 4KB.Thus, 4096 is no longer accepted as a valid value, despite still being the default for
udp_wmem_min, and for 'min' intcp_wmem. A huge number of sysctl configurations at FB use 4096 as 'min', so this change breaks all of them.This patch changes the sysctls to simply enforce that the value written is greater than or equal to the default value of
SK_MEM_QUANTUM.
Quoted 7 lines
Quoted 2 lines
Fixes 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")Signed-off-by Calvin Owens <[email protected]>I think increasing the default makes more sense.
If we don't allow applications to set 4K, the kernel shouldn't start with that value either.
I'm really questioning the limitation itself: why enforce a minimum of SOCK_MIN_SNDBUF here? Why not SK_MEM_QUANTUM?
Commit 8133534c760d4083 referred to b1cb59cf2efe7971, which choose to use the SOCK_MIN constants as the lower limits to avoid nasty bugs. But AFAICS, a limit of SOCK_MIN_SNDBUF isn't necessary to do that: the BUG_ON cited in the commit message for b1cb59cf2efe7971 seems to have happened because unix_stream_sendmsg() expects a minimum of a full page (ie SK_MEM_QUANTUM) and the math broke, not because it had less than SOCK_MIN_SNDBUF allocated.
Nothing seems to assume that it has at least SOCK_MIN_SNDBUF to play with, so my argument is that enforcing a minimum of SK_MEM_QUANTUM avoids the sort of bugs commit 8133534c760d4083 was trying to avoid, and it does so without breaking anybody's sysctl configurations. What do you think?
Thanks very much,
Calvin
David Miller
maintainerFrom: Calvin Owens <[email protected]>
Date: Mon, 10 Aug 2015 20:34:06 -0700
Quoted 11 lines
I'm really questioning the limitation itself: why enforce a minimum of
SOCK_MIN_SNDBUFhere? Why notSK_MEM_QUANTUM?Commit
8133534c760d4083referred tob1cb59cf2efe7971, which choose to use theSOCK_MINconstants as the lower limits to avoid nasty bugs. But AFAICS, a limit ofSOCK_MIN_SNDBUFisn't necessary to do that: theBUG_ONcited in the commit message forb1cb59cf2efe7971seems to have happened becauseunix_stream_sendmsg()expects a minimum of a full page (ieSK_MEM_QUANTUM) and the math broke, not because it had less thanSOCK_MIN_SNDBUFallocated.
Quoted 5 lines
Nothing seems to assume that it has at least
SOCK_MIN_SNDBUFto play with, so my argument is that enforcing a minimum ofSK_MEM_QUANTUMavoids the sort of bugs commit8133534c760d4083was trying to avoid, and it does so without breaking anybody's sysctl configurations. What do you think?
The author of said commit argues that too small values lead to really bad performance, but I guess he should have adjusted the default if he cared about it so much.
Ok, can you respin your patch with some added details in the commit message like what you said above?
Thanks.
PATCH 1/1 net: Use SK_MEM_QUANTUM as minimum for tcp/udp rmem/wmem
Commit 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values written to them are not less than SOCK_MIN_{RCV,SND}BUF.
That change causes 4096 (or SK_MEM_QUANTUM) to no longer be accepted as a valid value for 'min' in tcp_wmem and udp_wmem_min. 4096 has been the default for both of those sysctls for a long time, and unfortunately seems to be an extremely popular setting. This change breaks a large number of sysctl configurations at FB.
That commit referred to b1cb59cf2efe7971 ("net: sysctl_net_core: check SNDBUF and RCVBUF for min length"), which choose to use the SOCK_MIN constants as the lower limits to avoid nasty bugs. But AFAICS, a limit of SOCK_MIN_SNDBUF isn't necessary to do that: the BUG_ON cited in the commit message seems to have happened because unix_stream_sendmsg() expects a minimum of a full page (ie SK_MEM_QUANTUM) and the math broke, not because it had less than SOCK_MIN_SNDBUF allocated.
Nothing seems to assume that it has at least SOCK_MIN_SNDBUF to play with, so I think enforcing a minimum of SK_MEM_QUANTUM avoids the sort of bugs 8133534c was trying to avoid, and it does so without breaking anybody's sysctl configurations.
--- net/ipv4/sysctl_net_ipv4.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
net/ipv4/sysctl_net_ipv4.c +5 -6
@@ -41,8 +41,7 @@ static int tcp_syn_retries_min = 1;static int tcp_syn_retries_max =MAX_TCP_SYNCNT ;static int ip_ping_group_range_min[] = {0 ,0 };static int ip_ping_group_range_max[] = {GID_T_MAX ,GID_T_MAX }; -static int min_sndbuf =SOCK_MIN_SNDBUF ; -static int min_rcvbuf =SOCK_MIN_RCVBUF ; +static int min_buf =SK_MEM_QUANTUM ;/* Update system visible IP port range */ static void set_local_port_range (struct net *net,int range[2 ])
@@ -530,7 +529,7 @@ static struct ctl_table ipv4_table[] = { .maxlen = sizeof(sysctl_tcp_wmem), .mode = 0644, .proc_handler = proc_dointvec_minmax, - .extra1 = &min_sndbuf, + .extra1 = &min_buf, }, { .procname = "tcp_notsent_lowat",
@@ -545,7 +544,7 @@ static struct ctl_table ipv4_table[] = { .maxlen = sizeof(sysctl_tcp_rmem), .mode = 0644, .proc_handler = proc_dointvec_minmax, - .extra1 = &min_rcvbuf, + .extra1 = &min_buf, }, { .procname = "tcp_app_win",
@@ -758,7 +757,7 @@ static struct ctl_table ipv4_table[] = { .maxlen = sizeof(sysctl_udp_rmem_min), .mode = 0644, .proc_handler = proc_dointvec_minmax, - .extra1 = &min_rcvbuf, + .extra1 = &min_buf, }, { .procname = "udp_wmem_min",
@@ -766,7 +765,7 @@ static struct ctl_table ipv4_table[] = { .maxlen = sizeof(sysctl_udp_wmem_min), .mode = 0644, .proc_handler = proc_dointvec_minmax, - .extra1 = &min_sndbuf, + .extra1 = &min_buf, }, { } };
Signature · 1 line
1.8.1
Re: [PATCH] net: Use SK_MEM_QUANTUM as minimum for tcp/udp rmem/wmem
Quoted 18 lines · Calvin Owens
Commit
8133534c760d4083("net: limit tcp/udp rmem/wmem to SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values written to them are not less than SOCK_MIN_{RCV,SND}BUF.That change causes 4096 (or
SK_MEM_QUANTUM) to no longer be accepted as a valid value for 'min' intcp_wmemandudp_wmem_min. 4096 has been the default for both of those sysctls for a long time, and unfortunately seems to be an extremely popular setting. This change breaks a large number of sysctl configurations at FB.That commit referred to
b1cb59cf2efe7971("net:sysctl_net_core: check SNDBUF and RCVBUF for min length"), which choose to use theSOCK_MINconstants as the lower limits to avoid nasty bugs. But AFAICS, a limit ofSOCK_MIN_SNDBUFisn't necessary to do that: theBUG_ONcited in the commit message seems to have happened becauseunix_stream_sendmsg()expects a minimum of a full page (ieSK_MEM_QUANTUM) and the math broke, not because it had less thanSOCK_MIN_SNDBUFallocated.
Quoted 8 lines
Nothing seems to assume that it has at least
SOCK_MIN_SNDBUFto play with, so I think enforcing a minimum ofSK_MEM_QUANTUMavoids the sort of bugs8133534cwas trying to avoid, and it does so without breaking anybody's sysctl configurations.Fixes 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")Signed-off-by Calvin Owens <[email protected]>---
#define SK_MEM_QUANTUM ((int)PAGE_SIZE)
Some arches have PAGE_SIZE = 65536
So your patch might break scripts as well for them.
We should revert 8133534c760d4083.
Re: [PATCH] net: Use SK_MEM_QUANTUM as minimum for tcp/udp rmem/wmem
Quoted 28 lines · Eric Dumazet
Quoted 18 lines · Calvin Owens
Commit
8133534c760d4083("net: limit tcp/udp rmem/wmem to SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values written to them are not less than SOCK_MIN_{RCV,SND}BUF.That change causes 4096 (or
SK_MEM_QUANTUM) to no longer be accepted as a valid value for 'min' intcp_wmemandudp_wmem_min. 4096 has been the default for both of those sysctls for a long time, and unfortunately seems to be an extremely popular setting. This change breaks a large number of sysctl configurations at FB.That commit referred to
b1cb59cf2efe7971("net:sysctl_net_core: check SNDBUF and RCVBUF for min length"), which choose to use theSOCK_MINconstants as the lower limits to avoid nasty bugs. But AFAICS, a limit ofSOCK_MIN_SNDBUFisn't necessary to do that: theBUG_ONcited in the commit message seems to have happened becauseunix_stream_sendmsg()expects a minimum of a full page (ieSK_MEM_QUANTUM) and the math broke, not because it had less thanSOCK_MIN_SNDBUFallocated.Quoted 8 lines
Nothing seems to assume that it has at least
SOCK_MIN_SNDBUFto play with, so I think enforcing a minimum ofSK_MEM_QUANTUMavoids the sort of bugs8133534cwas trying to avoid, and it does so without breaking anybody's sysctl configurations.Fixes 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")Signed-off-by Calvin Owens <[email protected]>---
Quoted 8 lines
#define
SK_MEM_QUANTUM((int)PAGE_SIZE)Some arches have
PAGE_SIZE= 65536So your patch might break scripts as well for them.
We should revert
8133534c760d4083.
Would clamping the values to a min value, like setsockopt(SO_SNDBUF) does, be an option?
I still find it odd that SO_SNDBUF limits you, while the /proc interface doesn't. If you think it's
too much, I'm ok with reverting it since it affects scripts.
On those arches where PAGE_SIZE == 64K(or > 16K) it looks like we have tcp_wmem[1]
smaller than tcp_wmem[0]. Shouldn't we do something about this?
Re: [PATCH] net: Use SK_MEM_QUANTUM as minimum for tcp/udp rmem/wmem
Quoted 9 lines · Sorin Dumitru
Would clamping the values to a min value, like
setsockopt(SO_SNDBUF)does, be an option?
I still find it odd thatSO_SNDBUFlimits you, while the /proc interface doesn't. If you think it's
too much, I'm ok with reverting it since it affects scripts.On those arches where
PAGE_SIZE== 64K(or > 16K) it looks like we have tcp_wmem[1]
smaller than tcp_wmem[0]. Shouldn't we do something about this?
As long as we do not crash if/when root user changes /proc/sys/net settings, we are good.
I would not care if performance is bad if root does something really stupid. root user is supposed to not mess things just for fun.
Re: [PATCH] net: Use SK_MEM_QUANTUM as minimum for tcp/udp rmem/wmem
Quoted 8 lines · Eric Dumazet
Quoted 6 lines · Sorin Dumitru
Would clamping the values to a min value, like
setsockopt(SO_SNDBUF)does, be an option?
I still find it odd thatSO_SNDBUFlimits you, while the /proc interface doesn't. If you think it's
too much, I'm ok with reverting it since it affects scripts.
Quoted 6 lines
Quoted 3 lines
On those arches where
PAGE_SIZE== 64K(or > 16K) it looks like we have tcp_wmem[1]
smaller than tcp_wmem[0]. Shouldn't we do something about this?As long as we do not crash if/when root user changes
/proc/sys/netsettings, we are good.
Using "1 1 1" for tcp_{r,w}mem seems not to explode, so this sounds good to me. I'll send a new patch reverting the original.
Thanks,
Calvin
Quoted 2 lines
I would not care if performance is bad if root does something really stupid. root user is supposed to not mess things just for fun.
PATCH 1/1 Revert "net: limit tcp/udp rmem/wmem to SOCK_{RCV,SND}BUF_MIN"
In reply to Calvin OwensCommit 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values written to them are not less than SOCK_MIN_{RCV,SND}BUF.
That change causes 4096 to no longer be accepted as a valid value for 'min' in tcp_wmem and udp_wmem_min. 4096 has been the default for both of those sysctls for a long time, and unfortunately seems to be an extremely popular setting. This change breaks a large number of sysctl configurations at Facebook.
That commit referred to b1cb59cf2efe7971 ("net: sysctl_net_core: check SNDBUF and RCVBUF for min length"), which choose to use the SOCK_MIN constants as the lower limits to avoid nasty bugs. But AFAICS, a limit of SOCK_MIN_SNDBUF isn't necessary to do that: the BUG_ON cited in the commit message seems to have happened because unix_stream_sendmsg() expects a minimum of a full page (ie SK_MEM_QUANTUM) and the math broke, not because it had less than SOCK_MIN_SNDBUF allocated.
This particular issue doesn't seem to affect TCP however: using a setting of "1 1 1" for tcp_{r,w}mem works, although it's obviously suboptimal. SK_MEM_QUANTUM would be a nice minimum, but it's 64K on some archs, so there would still be breakage.
Since a value of one doesn't seem to cause any problems, we can drop the minimum 8133534c added to fix this.
This reverts commit 8133534c760d4083f79d2cde42c636ccc0b2792e.
--- net/ipv4/sysctl_net_ipv4.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
net/ipv4/sysctl_net_ipv4.c +4 -6
@@ -41,8 +41,6 @@ static int tcp_syn_retries_min = 1;static int tcp_syn_retries_max =MAX_TCP_SYNCNT ;static int ip_ping_group_range_min[] = {0 ,0 };static int ip_ping_group_range_max[] = {GID_T_MAX ,GID_T_MAX }; -static int min_sndbuf =SOCK_MIN_SNDBUF ; -static int min_rcvbuf =SOCK_MIN_RCVBUF ;/* Update system visible IP port range */ static void set_local_port_range (struct net *net,int range[2 ])
@@ -530,7 +528,7 @@ static struct ctl_table ipv4_table[] = { .maxlen = sizeof(sysctl_tcp_wmem), .mode = 0644, .proc_handler = proc_dointvec_minmax, - .extra1 = &min_sndbuf, + .extra1 = &one, }, { .procname = "tcp_notsent_lowat",
@@ -545,7 +543,7 @@ static struct ctl_table ipv4_table[] = { .maxlen = sizeof(sysctl_tcp_rmem), .mode = 0644, .proc_handler = proc_dointvec_minmax, - .extra1 = &min_rcvbuf, + .extra1 = &one, }, { .procname = "tcp_app_win",
@@ -758,7 +756,7 @@ static struct ctl_table ipv4_table[] = { .maxlen = sizeof(sysctl_udp_rmem_min), .mode = 0644, .proc_handler = proc_dointvec_minmax, - .extra1 = &min_rcvbuf, + .extra1 = &one }, { .procname = "udp_wmem_min",
@@ -766,7 +764,7 @@ static struct ctl_table ipv4_table[] = { .maxlen = sizeof(sysctl_udp_wmem_min), .mode = 0644, .proc_handler = proc_dointvec_minmax, - .extra1 = &min_sndbuf, + .extra1 = &one }, { } };
Signature · 1 line
2.5.0
Re: [PATCH] Revert "net: limit tcp/udp rmem/wmem to SOCK_{RCV,SND}BUF_MIN"
Quoted 4 lines · Calvin Owens
Commit
8133534c760d4083("net: limit tcp/udp rmem/wmem to SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values written to them are not less than SOCK_MIN_{RCV,SND}BUF.
...
Quoted 7 lines
This reverts commit
8133534c760d4083f79d2cde42c636ccc0b2792e.Fixes 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")Cc Eric Dumazet <[email protected]>Cc Sorin Dumitru <[email protected]>Signed-off-by Calvin Owens <[email protected]>
Re: [PATCH] Revert "net: limit tcp/udp rmem/wmem to SOCK_{RCV,SND}BUF_MIN"
In reply to Calvin OwensFrom: Calvin Owens <[email protected]>
Date: Thu, 13 Aug 2015 14:21:34 -0700
Quoted 26 lines
Commit
8133534c760d4083("net: limit tcp/udp rmem/wmem to SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values written to them are not less than SOCK_MIN_{RCV,SND}BUF.That change causes 4096 to no longer be accepted as a valid value for 'min' in
tcp_wmemandudp_wmem_min. 4096 has been the default for both of those sysctls for a long time, and unfortunately seems to be an extremely popular setting. This change breaks a large number of sysctl configurations at Facebook.That commit referred to
b1cb59cf2efe7971("net:sysctl_net_core: check SNDBUF and RCVBUF for min length"), which choose to use theSOCK_MINconstants as the lower limits to avoid nasty bugs. But AFAICS, a limit ofSOCK_MIN_SNDBUFisn't necessary to do that: theBUG_ONcited in the commit message seems to have happened becauseunix_stream_sendmsg()expects a minimum of a full page (ieSK_MEM_QUANTUM) and the math broke, not because it had less thanSOCK_MIN_SNDBUFallocated.This particular issue doesn't seem to affect TCP however: using a setting of "1 1 1" for tcp_{r,w}mem works, although it's obviously suboptimal.
SK_MEM_QUANTUMwould be a nice minimum, but it's 64K on some archs, so there would still be breakage.Since a value of one doesn't seem to cause any problems, we can drop the minimum
8133534cadded to fix this.
Quoted 6 lines
This reverts commit
8133534c760d4083f79d2cde42c636ccc0b2792e.Fixes 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")Cc Eric Dumazet <[email protected]>Cc Sorin Dumitru <[email protected]>Signed-off-by Calvin Owens <[email protected]>
Applied, thanks.