[PATCH net-next 0/2] tcp: add sysctl_tcp_rto_min_us

[PATCH net-next 0/2] tcp: add sysctl_tcp_rto_min_us

[Kevin Yang]

Adding a sysctl knob to allow user to specify a default
rto_min at socket init time.

After this patch series, the rto_min will has multiple sources:
route option has the highest precedence, followed by the
TCP_BPF_RTO_MIN socket option, followed by this new
tcp_rto_min_us sysctl.

Kevin Yang (2):
  tcp: derive delack_max with tcp_rto_min helper
  tcp: add sysctl_tcp_rto_min_us

 Documentation/networking/ip-sysctl.rst | 13 +++++++++++++
 include/net/netns/ipv4.h               |  1 +
 net/ipv4/sysctl_net_ipv4.c             |  8 ++++++++
 net/ipv4/tcp.c                         |  3 ++-
 net/ipv4/tcp_ipv4.c                    |  1 +
 net/ipv4/tcp_output.c                  | 11 ++---------
 6 files changed, 27 insertions(+), 10 deletions(-)

-- 
2.45.1.288.g0e0cd299f1-goog

[PATCH net-next 1/2] tcp: derive delack_max with tcp_rto_min helper

[Kevin Yang]

Rto_min now has multiple souces, ordered by preprecedence high to
low: ip route option rto_min, icsk->icsk_rto_min.

When derive delack_max from rto_min, we should not only use ip
route option, but should use tcp_rto_min helper to get the correct
rto_min.

Signed-off-by: Kevin Yang <yyd@google.com>
Reviewed-by: Neal Cardwell <ncardwell@google.com>
Reviewed-by: Yuchung Cheng <ycheng@google.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_output.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 95caf8aaa8be..c90362c1f724 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -4163,16 +4163,9 @@ EXPORT_SYMBOL(tcp_connect);
 
 u32 tcp_delack_max(const struct sock *sk)
 {
-	const struct dst_entry *dst = __sk_dst_get(sk);
-	u32 delack_max = inet_csk(sk)->icsk_delack_max;
-
-	if (dst && dst_metric_locked(dst, RTAX_RTO_MIN)) {
-		u32 rto_min = dst_metric_rtt(dst, RTAX_RTO_MIN);
-		u32 delack_from_rto_min = max_t(int, 1, rto_min - 1);
+	u32 delack_from_rto_min = max_t(int, 1, tcp_rto_min(sk) - 1);
 
-		delack_max = min_t(u32, delack_max, delack_from_rto_min);
-	}
-	return delack_max;
+	return min_t(u32, inet_csk(sk)->icsk_delack_max, delack_from_rto_min);
 }
 
 /* Send out a delayed ack, the caller does the policy checking
-- 
2.45.1.288.g0e0cd299f1-goog

[PATCH net-next 2/2] tcp: add sysctl_tcp_rto_min_us

[Kevin Yang]

Adding a sysctl knob to allow user to specify a default
rto_min at socket init time, other than using the hard
coded 200ms default rto_min.

Note that the rto_min route option has the highest precedence
for configuring this setting, followed by the TCP_BPF_RTO_MIN
socket option, followed by the tcp_rto_min_us sysctl.

Signed-off-by: Kevin Yang <yyd@google.com>
Reviewed-by: Neal Cardwell <ncardwell@google.com>
Reviewed-by: Yuchung Cheng <ycheng@google.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
 Documentation/networking/ip-sysctl.rst | 13 +++++++++++++
 include/net/netns/ipv4.h               |  1 +
 net/ipv4/sysctl_net_ipv4.c             |  8 ++++++++
 net/ipv4/tcp.c                         |  3 ++-
 net/ipv4/tcp_ipv4.c                    |  1 +
 5 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index bd50df6a5a42..6e99eccdb837 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -1196,6 +1196,19 @@ tcp_pingpong_thresh - INTEGER
 
 	Default: 1
 
+tcp_rto_min_us - INTEGER
+	Minimal TCP retransmission timeout (in microseconds). Note that the
+	rto_min route option has the highest precedence for configuring this
+	setting, followed by the TCP_BPF_RTO_MIN socket option, followed by
+	this tcp_rto_min_us sysctl.
+
+	The recommended practice is to use a value less or equal to 200000
+	microseconds.
+
+	Possible Values: 1 - INT_MAX
+
+	Default: 200000
+
 UDP variables
 =============
 
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index c356c458b340..a91bb971f901 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -170,6 +170,7 @@ struct netns_ipv4 {
 	u8 sysctl_tcp_sack;
 	u8 sysctl_tcp_window_scaling;
 	u8 sysctl_tcp_timestamps;
+	int sysctl_tcp_rto_min_us;
 	u8 sysctl_tcp_recovery;
 	u8 sysctl_tcp_thin_linear_timeouts;
 	u8 sysctl_tcp_slow_start_after_idle;
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 162a0a3b6ba5..58be05f8812c 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -1501,6 +1501,14 @@ static struct ctl_table ipv4_net_table[] = {
 		.proc_handler	= proc_dou8vec_minmax,
 		.extra1		= SYSCTL_ONE,
 	},
+	{
+		.procname	= "tcp_rto_min_us",
+		.data		= &init_net.ipv4.sysctl_tcp_rto_min_us,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ONE,
+	},
 };
 
 static __net_init int ipv4_sysctl_init_net(struct net *net)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 06aab937d60a..8e91b60ac1ce 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -428,7 +428,8 @@ void tcp_init_sock(struct sock *sk)
 	INIT_LIST_HEAD(&tp->tsorted_sent_queue);
 
 	icsk->icsk_rto = TCP_TIMEOUT_INIT;
-	icsk->icsk_rto_min = TCP_RTO_MIN;
+	icsk->icsk_rto_min = usecs_to_jiffies(READ_ONCE(sock_net(sk)->
+					      ipv4.sysctl_tcp_rto_min_us));
 	icsk->icsk_delack_max = TCP_DELACK_MAX;
 	tp->mdev_us = jiffies_to_usecs(TCP_TIMEOUT_INIT);
 	minmax_reset(&tp->rtt_min, tcp_jiffies32, ~0U);
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 108a438dc247..da005a197ca1 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -3511,6 +3511,7 @@ static int __net_init tcp_sk_init(struct net *net)
 	net->ipv4.sysctl_tcp_shrink_window = 0;
 
 	net->ipv4.sysctl_tcp_pingpong_thresh = 1;
+	net->ipv4.sysctl_tcp_rto_min_us = jiffies_to_usecs(TCP_RTO_MIN);
 
 	return 0;
 }
-- 
2.45.1.288.g0e0cd299f1-goog

Re: [PATCH net-next 0/2] tcp: add sysctl_tcp_rto_min_us

[Jason Xing]

Hello Kevin,

On Wed, May 29, 2024 at 1:13 AM Kevin Yang <yyd@google.com> wrote:
>
> Adding a sysctl knob to allow user to specify a default
> rto_min at socket init time.

I wonder what the advantage of this new sysctl knob is since we have
had BPF or something like that to tweak the rto min already?

There are so many places/parameters of the TCP stack that can be
exposed to the user side and adjusted by new sysctls...

Thanks,
Jason

>
> After this patch series, the rto_min will has multiple sources:
> route option has the highest precedence, followed by the
> TCP_BPF_RTO_MIN socket option, followed by this new
> tcp_rto_min_us sysctl.
>
> Kevin Yang (2):
>   tcp: derive delack_max with tcp_rto_min helper
>   tcp: add sysctl_tcp_rto_min_us
>
>  Documentation/networking/ip-sysctl.rst | 13 +++++++++++++
>  include/net/netns/ipv4.h               |  1 +
>  net/ipv4/sysctl_net_ipv4.c             |  8 ++++++++
>  net/ipv4/tcp.c                         |  3 ++-
>  net/ipv4/tcp_ipv4.c                    |  1 +
>  net/ipv4/tcp_output.c                  | 11 ++---------
>  6 files changed, 27 insertions(+), 10 deletions(-)
>
> --
> 2.45.1.288.g0e0cd299f1-goog
>
>

Re: [PATCH net-next 0/2] tcp: add sysctl_tcp_rto_min_us

[Jason Xing]

On Wed, May 29, 2024 at 2:43 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> Hello Kevin,
>
> On Wed, May 29, 2024 at 1:13 AM Kevin Yang <yyd@google.com> wrote:
> >
> > Adding a sysctl knob to allow user to specify a default
> > rto_min at socket init time.
>
> I wonder what the advantage of this new sysctl knob is since we have
> had BPF or something like that to tweak the rto min already?
>
> There are so many places/parameters of the TCP stack that can be
> exposed to the user side and adjusted by new sysctls...
>
> Thanks,
> Jason
>
> >
> > After this patch series, the rto_min will has multiple sources:
> > route option has the highest precedence, followed by the
> > TCP_BPF_RTO_MIN socket option, followed by this new
> > tcp_rto_min_us sysctl.
> >
> > Kevin Yang (2):
> >   tcp: derive delack_max with tcp_rto_min helper
> >   tcp: add sysctl_tcp_rto_min_us
> >
> >  Documentation/networking/ip-sysctl.rst | 13 +++++++++++++
> >  include/net/netns/ipv4.h               |  1 +
> >  net/ipv4/sysctl_net_ipv4.c             |  8 ++++++++
> >  net/ipv4/tcp.c                         |  3 ++-
> >  net/ipv4/tcp_ipv4.c                    |  1 +
> >  net/ipv4/tcp_output.c                  | 11 ++---------
> >  6 files changed, 27 insertions(+), 10 deletions(-)
> >
> > --
> > 2.45.1.288.g0e0cd299f1-goog
> >
> >

Oh, I think you should have added Paolo as well.

+Paolo Abeni

Re: [PATCH net-next 0/2] tcp: add sysctl_tcp_rto_min_us

[Tony Lu]

On Tue, May 28, 2024 at 05:13:18PM +0000, Kevin Yang wrote:
> Adding a sysctl knob to allow user to specify a default
> rto_min at socket init time.
> 
> After this patch series, the rto_min will has multiple sources:
> route option has the highest precedence, followed by the
> TCP_BPF_RTO_MIN socket option, followed by this new
> tcp_rto_min_us sysctl.

For series:

Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>

I strongly support those patches. For those who use cgroup v1 and want
to take effect with simple settings, sysctl is a good way.

And reducing it is helpful for latency-sensitive applications such as
Redis, net namespace level sysctl knob is enough.

> 
> Kevin Yang (2):
>   tcp: derive delack_max with tcp_rto_min helper
>   tcp: add sysctl_tcp_rto_min_us
> 
>  Documentation/networking/ip-sysctl.rst | 13 +++++++++++++
>  include/net/netns/ipv4.h               |  1 +
>  net/ipv4/sysctl_net_ipv4.c             |  8 ++++++++
>  net/ipv4/tcp.c                         |  3 ++-
>  net/ipv4/tcp_ipv4.c                    |  1 +
>  net/ipv4/tcp_output.c                  | 11 ++---------
>  6 files changed, 27 insertions(+), 10 deletions(-)
> 
> -- 
> 2.45.1.288.g0e0cd299f1-goog
> 

Re: [PATCH net-next 0/2] tcp: add sysctl_tcp_rto_min_us

[Eric Dumazet]

On Wed, May 29, 2024 at 9:00 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> On Wed, May 29, 2024 at 2:43 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > Hello Kevin,
> >
> > On Wed, May 29, 2024 at 1:13 AM Kevin Yang <yyd@google.com> wrote:
> > >
> > > Adding a sysctl knob to allow user to specify a default
> > > rto_min at socket init time.
> >
> > I wonder what the advantage of this new sysctl knob is since we have
> > had BPF or something like that to tweak the rto min already?
> >
> > There are so many places/parameters of the TCP stack that can be
> > exposed to the user side and adjusted by new sysctls...
> >
> > Thanks,
> > Jason
> >
> > >
> > > After this patch series, the rto_min will has multiple sources:
> > > route option has the highest precedence, followed by the
> > > TCP_BPF_RTO_MIN socket option, followed by this new
> > > tcp_rto_min_us sysctl.
> > >
> > > Kevin Yang (2):
> > >   tcp: derive delack_max with tcp_rto_min helper
> > >   tcp: add sysctl_tcp_rto_min_us
> > >
> > >  Documentation/networking/ip-sysctl.rst | 13 +++++++++++++
> > >  include/net/netns/ipv4.h               |  1 +
> > >  net/ipv4/sysctl_net_ipv4.c             |  8 ++++++++
> > >  net/ipv4/tcp.c                         |  3 ++-
> > >  net/ipv4/tcp_ipv4.c                    |  1 +
> > >  net/ipv4/tcp_output.c                  | 11 ++---------
> > >  6 files changed, 27 insertions(+), 10 deletions(-)
> > >
> > > --
> > > 2.45.1.288.g0e0cd299f1-goog
> > >
> > >
>
> Oh, I think you should have added Paolo as well.
>
> +Paolo Abeni

Many cloud customers do not have any BPF expertise.
If they use existing BPF programs (added by a product), they might not
have the ability to change it.

We tried advising them to use route attributes, after
commit bbf80d713fe75cfbecda26e7c03a9a8d22af2f4f ("tcp: derive
delack_max from rto_min")

Alas, dhcpd was adding its own routes, without the "rto_min 5"
attribute, then systemd came...
Lots of frustration, lots of wasted time, for something that has been
used for more than a decade
in Google DC.

With a sysctl, we could have saved months of SWE, and helped our
customers sooner.

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next 0/2] tcp: add sysctl_tcp_rto_min_us

[Tony Lu]

On Wed, May 29, 2024 at 09:39:02AM +0200, Eric Dumazet wrote:
> On Wed, May 29, 2024 at 9:00 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > On Wed, May 29, 2024 at 2:43 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > >
> > > Hello Kevin,
> > >
> > > On Wed, May 29, 2024 at 1:13 AM Kevin Yang <yyd@google.com> wrote:
> > > >
> > > > Adding a sysctl knob to allow user to specify a default
> > > > rto_min at socket init time.
> > >
> > > I wonder what the advantage of this new sysctl knob is since we have
> > > had BPF or something like that to tweak the rto min already?
> > >
> > > There are so many places/parameters of the TCP stack that can be
> > > exposed to the user side and adjusted by new sysctls...
> > >
> > > Thanks,
> > > Jason
> > >
> > > >
> > > > After this patch series, the rto_min will has multiple sources:
> > > > route option has the highest precedence, followed by the
> > > > TCP_BPF_RTO_MIN socket option, followed by this new
> > > > tcp_rto_min_us sysctl.
> > > >
> > > > Kevin Yang (2):
> > > >   tcp: derive delack_max with tcp_rto_min helper
> > > >   tcp: add sysctl_tcp_rto_min_us
> > > >
> > > >  Documentation/networking/ip-sysctl.rst | 13 +++++++++++++
> > > >  include/net/netns/ipv4.h               |  1 +
> > > >  net/ipv4/sysctl_net_ipv4.c             |  8 ++++++++
> > > >  net/ipv4/tcp.c                         |  3 ++-
> > > >  net/ipv4/tcp_ipv4.c                    |  1 +
> > > >  net/ipv4/tcp_output.c                  | 11 ++---------
> > > >  6 files changed, 27 insertions(+), 10 deletions(-)
> > > >
> > > > --
> > > > 2.45.1.288.g0e0cd299f1-goog
> > > >
> > > >
> >
> > Oh, I think you should have added Paolo as well.
> >
> > +Paolo Abeni
> 
> Many cloud customers do not have any BPF expertise.
> If they use existing BPF programs (added by a product), they might not
> have the ability to change it.

+1, eBPF actually is not easy to write, debug and manage for now.
Sysctls are easy to use, just put it into /etc/sysctl.conf and save it
into users' customized images or templates. AFAIK, there is no standard
system kit to handle eBPF in most OS distros.

> 
> We tried advising them to use route attributes, after
> commit bbf80d713fe75cfbecda26e7c03a9a8d22af2f4f ("tcp: derive
> delack_max from rto_min")
> 
> Alas, dhcpd was adding its own routes, without the "rto_min 5"
> attribute, then systemd came...
> Lots of frustration, lots of wasted time, for something that has been
> used for more than a decade
> in Google DC.
> 
> With a sysctl, we could have saved months of SWE, and helped our
> customers sooner.
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next 0/2] tcp: add sysctl_tcp_rto_min_us

[Jason Xing]

Hello Eric,

On Wed, May 29, 2024 at 3:39 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, May 29, 2024 at 9:00 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > On Wed, May 29, 2024 at 2:43 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > >
> > > Hello Kevin,
> > >
> > > On Wed, May 29, 2024 at 1:13 AM Kevin Yang <yyd@google.com> wrote:
> > > >
> > > > Adding a sysctl knob to allow user to specify a default
> > > > rto_min at socket init time.
> > >
> > > I wonder what the advantage of this new sysctl knob is since we have
> > > had BPF or something like that to tweak the rto min already?
> > >
> > > There are so many places/parameters of the TCP stack that can be
> > > exposed to the user side and adjusted by new sysctls...
> > >
> > > Thanks,
> > > Jason
> > >
> > > >
> > > > After this patch series, the rto_min will has multiple sources:
> > > > route option has the highest precedence, followed by the
> > > > TCP_BPF_RTO_MIN socket option, followed by this new
> > > > tcp_rto_min_us sysctl.
> > > >
> > > > Kevin Yang (2):
> > > >   tcp: derive delack_max with tcp_rto_min helper
> > > >   tcp: add sysctl_tcp_rto_min_us
> > > >
> > > >  Documentation/networking/ip-sysctl.rst | 13 +++++++++++++
> > > >  include/net/netns/ipv4.h               |  1 +
> > > >  net/ipv4/sysctl_net_ipv4.c             |  8 ++++++++
> > > >  net/ipv4/tcp.c                         |  3 ++-
> > > >  net/ipv4/tcp_ipv4.c                    |  1 +
> > > >  net/ipv4/tcp_output.c                  | 11 ++---------
> > > >  6 files changed, 27 insertions(+), 10 deletions(-)
> > > >
> > > > --
> > > > 2.45.1.288.g0e0cd299f1-goog
> > > >
> > > >
> >
> > Oh, I think you should have added Paolo as well.
> >
> > +Paolo Abeni
>
> Many cloud customers do not have any BPF expertise.
> If they use existing BPF programs (added by a product), they might not
> have the ability to change it.
>
> We tried advising them to use route attributes, after
> commit bbf80d713fe75cfbecda26e7c03a9a8d22af2f4f ("tcp: derive
> delack_max from rto_min")
>
> Alas, dhcpd was adding its own routes, without the "rto_min 5"
> attribute, then systemd came...
> Lots of frustration, lots of wasted time, for something that has been
> used for more than a decade
> in Google DC.
>
> With a sysctl, we could have saved months of SWE, and helped our
> customers sooner.

I'm definitely aware of the importance of this kind of sysctl knob.
Many years ago (around 6 or 7 years ago), we already implemented
similar things in the private kernel.

For a long time, netdev guys often proposed the question as I did in
the previous email. I'm not against it, just repeating the same
question and asking ourselves again: is it really necessary? We still
have a lot of places to tune/control by introducing new sysctl.

For a long time, there have been plenty of papers studying different
combinations of different parameters in TCP stack so that they can
serve one particular case well.

Do we also need to expose remaining possible parameters to the user
side? Just curious...

Thanks,
Jason

Re: [PATCH net-next 0/2] tcp: add sysctl_tcp_rto_min_us

[Jason Xing]

On Wed, May 29, 2024 at 3:21 PM Tony Lu <tonylu@linux.alibaba.com> wrote:
>
> On Tue, May 28, 2024 at 05:13:18PM +0000, Kevin Yang wrote:
> > Adding a sysctl knob to allow user to specify a default
> > rto_min at socket init time.
> >
> > After this patch series, the rto_min will has multiple sources:
> > route option has the highest precedence, followed by the
> > TCP_BPF_RTO_MIN socket option, followed by this new
> > tcp_rto_min_us sysctl.
>
> For series:
>
> Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
>
> I strongly support those patches. For those who use cgroup v1 and want
> to take effect with simple settings, sysctl is a good way.

It's not a good reason to use sysctl.

If you say so, why not introduce many sysctls to replace setsockopt
operations. For example, introducing a new sysctl to disable delayed
ack to improve the speed of transmission in some cases just for ease
of use? No, it's not right, I believe.

>
> And reducing it is helpful for latency-sensitive applications such as
> Redis, net namespace level sysctl knob is enough.

Sure, these key parameters play a big role in the TCP stack.

>
> >
> > Kevin Yang (2):
> >   tcp: derive delack_max with tcp_rto_min helper
> >   tcp: add sysctl_tcp_rto_min_us
> >
> >  Documentation/networking/ip-sysctl.rst | 13 +++++++++++++
> >  include/net/netns/ipv4.h               |  1 +
> >  net/ipv4/sysctl_net_ipv4.c             |  8 ++++++++
> >  net/ipv4/tcp.c                         |  3 ++-
> >  net/ipv4/tcp_ipv4.c                    |  1 +
> >  net/ipv4/tcp_output.c                  | 11 ++---------
> >  6 files changed, 27 insertions(+), 10 deletions(-)
> >
> > --
> > 2.45.1.288.g0e0cd299f1-goog
> >
>

Re: [PATCH net-next 0/2] tcp: add sysctl_tcp_rto_min_us

[Eric Dumazet]

On Wed, May 29, 2024 at 10:44 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> Hello Eric,
>
> On Wed, May 29, 2024 at 3:39 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Wed, May 29, 2024 at 9:00 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > >
> > > On Wed, May 29, 2024 at 2:43 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > > >
> > > > Hello Kevin,
> > > >
> > > > On Wed, May 29, 2024 at 1:13 AM Kevin Yang <yyd@google.com> wrote:
> > > > >
> > > > > Adding a sysctl knob to allow user to specify a default
> > > > > rto_min at socket init time.
> > > >
> > > > I wonder what the advantage of this new sysctl knob is since we have
> > > > had BPF or something like that to tweak the rto min already?
> > > >
> > > > There are so many places/parameters of the TCP stack that can be
> > > > exposed to the user side and adjusted by new sysctls...
> > > >
> > > > Thanks,
> > > > Jason
> > > >
> > > > >
> > > > > After this patch series, the rto_min will has multiple sources:
> > > > > route option has the highest precedence, followed by the
> > > > > TCP_BPF_RTO_MIN socket option, followed by this new
> > > > > tcp_rto_min_us sysctl.
> > > > >
> > > > > Kevin Yang (2):
> > > > >   tcp: derive delack_max with tcp_rto_min helper
> > > > >   tcp: add sysctl_tcp_rto_min_us
> > > > >
> > > > >  Documentation/networking/ip-sysctl.rst | 13 +++++++++++++
> > > > >  include/net/netns/ipv4.h               |  1 +
> > > > >  net/ipv4/sysctl_net_ipv4.c             |  8 ++++++++
> > > > >  net/ipv4/tcp.c                         |  3 ++-
> > > > >  net/ipv4/tcp_ipv4.c                    |  1 +
> > > > >  net/ipv4/tcp_output.c                  | 11 ++---------
> > > > >  6 files changed, 27 insertions(+), 10 deletions(-)
> > > > >
> > > > > --
> > > > > 2.45.1.288.g0e0cd299f1-goog
> > > > >
> > > > >
> > >
> > > Oh, I think you should have added Paolo as well.
> > >
> > > +Paolo Abeni
> >
> > Many cloud customers do not have any BPF expertise.
> > If they use existing BPF programs (added by a product), they might not
> > have the ability to change it.
> >
> > We tried advising them to use route attributes, after
> > commit bbf80d713fe75cfbecda26e7c03a9a8d22af2f4f ("tcp: derive
> > delack_max from rto_min")
> >
> > Alas, dhcpd was adding its own routes, without the "rto_min 5"
> > attribute, then systemd came...
> > Lots of frustration, lots of wasted time, for something that has been
> > used for more than a decade
> > in Google DC.
> >
> > With a sysctl, we could have saved months of SWE, and helped our
> > customers sooner.
>
> I'm definitely aware of the importance of this kind of sysctl knob.
> Many years ago (around 6 or 7 years ago), we already implemented
> similar things in the private kernel.
>
> For a long time, netdev guys often proposed the question as I did in
> the previous email. I'm not against it, just repeating the same
> question and asking ourselves again: is it really necessary? We still
> have a lot of places to tune/control by introducing new sysctl.
>
> For a long time, there have been plenty of papers studying different
> combinations of different parameters in TCP stack so that they can
> serve one particular case well.
>
> Do we also need to expose remaining possible parameters to the user
> side? Just curious...

You know, counting CLOSE_WAIT can be done with  eBPF program just fine.

I think long-time TCP maintainers like Eric Dumazet, Neal Cardwell,
and Yuchung Cheng know better,
you will have to trust us.

If you do not want to use the sysctl, this is fine, we do not force you.

Re: [PATCH net-next 0/2] tcp: add sysctl_tcp_rto_min_us

[Jason Xing]

On Wed, May 29, 2024 at 5:23 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, May 29, 2024 at 10:44 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > Hello Eric,
> >
> > On Wed, May 29, 2024 at 3:39 PM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Wed, May 29, 2024 at 9:00 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > > >
> > > > On Wed, May 29, 2024 at 2:43 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > > > >
> > > > > Hello Kevin,
> > > > >
> > > > > On Wed, May 29, 2024 at 1:13 AM Kevin Yang <yyd@google.com> wrote:
> > > > > >
> > > > > > Adding a sysctl knob to allow user to specify a default
> > > > > > rto_min at socket init time.
> > > > >
> > > > > I wonder what the advantage of this new sysctl knob is since we have
> > > > > had BPF or something like that to tweak the rto min already?
> > > > >
> > > > > There are so many places/parameters of the TCP stack that can be
> > > > > exposed to the user side and adjusted by new sysctls...
> > > > >
> > > > > Thanks,
> > > > > Jason
> > > > >
> > > > > >
> > > > > > After this patch series, the rto_min will has multiple sources:
> > > > > > route option has the highest precedence, followed by the
> > > > > > TCP_BPF_RTO_MIN socket option, followed by this new
> > > > > > tcp_rto_min_us sysctl.
> > > > > >
> > > > > > Kevin Yang (2):
> > > > > >   tcp: derive delack_max with tcp_rto_min helper
> > > > > >   tcp: add sysctl_tcp_rto_min_us
> > > > > >
> > > > > >  Documentation/networking/ip-sysctl.rst | 13 +++++++++++++
> > > > > >  include/net/netns/ipv4.h               |  1 +
> > > > > >  net/ipv4/sysctl_net_ipv4.c             |  8 ++++++++
> > > > > >  net/ipv4/tcp.c                         |  3 ++-
> > > > > >  net/ipv4/tcp_ipv4.c                    |  1 +
> > > > > >  net/ipv4/tcp_output.c                  | 11 ++---------
> > > > > >  6 files changed, 27 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > --
> > > > > > 2.45.1.288.g0e0cd299f1-goog
> > > > > >
> > > > > >
> > > >
> > > > Oh, I think you should have added Paolo as well.
> > > >
> > > > +Paolo Abeni
> > >
> > > Many cloud customers do not have any BPF expertise.
> > > If they use existing BPF programs (added by a product), they might not
> > > have the ability to change it.
> > >
> > > We tried advising them to use route attributes, after
> > > commit bbf80d713fe75cfbecda26e7c03a9a8d22af2f4f ("tcp: derive
> > > delack_max from rto_min")
> > >
> > > Alas, dhcpd was adding its own routes, without the "rto_min 5"
> > > attribute, then systemd came...
> > > Lots of frustration, lots of wasted time, for something that has been
> > > used for more than a decade
> > > in Google DC.
> > >
> > > With a sysctl, we could have saved months of SWE, and helped our
> > > customers sooner.
> >
> > I'm definitely aware of the importance of this kind of sysctl knob.
> > Many years ago (around 6 or 7 years ago), we already implemented
> > similar things in the private kernel.
> >
> > For a long time, netdev guys often proposed the question as I did in
> > the previous email. I'm not against it, just repeating the same
> > question and asking ourselves again: is it really necessary? We still
> > have a lot of places to tune/control by introducing new sysctl.
> >
> > For a long time, there have been plenty of papers studying different
> > combinations of different parameters in TCP stack so that they can
> > serve one particular case well.
> >
> > Do we also need to expose remaining possible parameters to the user
> > side? Just curious...
>
> You know, counting CLOSE_WAIT can be done with  eBPF program just fine.
>
> I think long-time TCP maintainers like Eric Dumazet, Neal Cardwell,
> and Yuchung Cheng know better,
> you will have to trust us.

You get me wrong, Eric. I trust you, of course. I'm just out of
curiosity because I saw some threads facing the same question before.

And, as I said, it has been used in our private kernel for a long
time. So it's useful.

BTW, what do you think of that close-wait patch since you mention it.

Thanks,
Jason

Re: [PATCH net-next 0/2] tcp: add sysctl_tcp_rto_min_us

[Kevin Yang]

Hello Jason,

I guess I don't have anymore to add than what Eric and Tony mentioned.

The purpose of this sysctl is to resolve problems in a more straightforward
way than other existing methods.

Thanks
kevin

On Wed, May 29, 2024 at 5:23 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, May 29, 2024 at 10:44 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > Hello Eric,
> >
> > On Wed, May 29, 2024 at 3:39 PM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Wed, May 29, 2024 at 9:00 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > > >
> > > > On Wed, May 29, 2024 at 2:43 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > > > >
> > > > > Hello Kevin,
> > > > >
> > > > > On Wed, May 29, 2024 at 1:13 AM Kevin Yang <yyd@google.com> wrote:
> > > > > >
> > > > > > Adding a sysctl knob to allow user to specify a default
> > > > > > rto_min at socket init time.
> > > > >
> > > > > I wonder what the advantage of this new sysctl knob is since we have
> > > > > had BPF or something like that to tweak the rto min already?
> > > > >
> > > > > There are so many places/parameters of the TCP stack that can be
> > > > > exposed to the user side and adjusted by new sysctls...
> > > > >
> > > > > Thanks,
> > > > > Jason
> > > > >
> > > > > >
> > > > > > After this patch series, the rto_min will has multiple sources:
> > > > > > route option has the highest precedence, followed by the
> > > > > > TCP_BPF_RTO_MIN socket option, followed by this new
> > > > > > tcp_rto_min_us sysctl.
> > > > > >
> > > > > > Kevin Yang (2):
> > > > > >   tcp: derive delack_max with tcp_rto_min helper
> > > > > >   tcp: add sysctl_tcp_rto_min_us
> > > > > >
> > > > > >  Documentation/networking/ip-sysctl.rst | 13 +++++++++++++
> > > > > >  include/net/netns/ipv4.h               |  1 +
> > > > > >  net/ipv4/sysctl_net_ipv4.c             |  8 ++++++++
> > > > > >  net/ipv4/tcp.c                         |  3 ++-
> > > > > >  net/ipv4/tcp_ipv4.c                    |  1 +
> > > > > >  net/ipv4/tcp_output.c                  | 11 ++---------
> > > > > >  6 files changed, 27 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > --
> > > > > > 2.45.1.288.g0e0cd299f1-goog
> > > > > >
> > > > > >
> > > >
> > > > Oh, I think you should have added Paolo as well.
> > > >
> > > > +Paolo Abeni
> > >
> > > Many cloud customers do not have any BPF expertise.
> > > If they use existing BPF programs (added by a product), they might not
> > > have the ability to change it.
> > >
> > > We tried advising them to use route attributes, after
> > > commit bbf80d713fe75cfbecda26e7c03a9a8d22af2f4f ("tcp: derive
> > > delack_max from rto_min")
> > >
> > > Alas, dhcpd was adding its own routes, without the "rto_min 5"
> > > attribute, then systemd came...
> > > Lots of frustration, lots of wasted time, for something that has been
> > > used for more than a decade
> > > in Google DC.
> > >
> > > With a sysctl, we could have saved months of SWE, and helped our
> > > customers sooner.
> >
> > I'm definitely aware of the importance of this kind of sysctl knob.
> > Many years ago (around 6 or 7 years ago), we already implemented
> > similar things in the private kernel.
> >
> > For a long time, netdev guys often proposed the question as I did in
> > the previous email. I'm not against it, just repeating the same
> > question and asking ourselves again: is it really necessary? We still
> > have a lot of places to tune/control by introducing new sysctl.
> >
> > For a long time, there have been plenty of papers studying different
> > combinations of different parameters in TCP stack so that they can
> > serve one particular case well.
> >
> > Do we also need to expose remaining possible parameters to the user
> > side? Just curious...
>
> You know, counting CLOSE_WAIT can be done with  eBPF program just fine.
>
> I think long-time TCP maintainers like Eric Dumazet, Neal Cardwell,
> and Yuchung Cheng know better,
> you will have to trust us.
>
> If you do not want to use the sysctl, this is fine, we do not force you.

Re: [PATCH net-next 2/2] tcp: add sysctl_tcp_rto_min_us

[Jakub Kicinski]

On Tue, 28 May 2024 17:13:20 +0000 Kevin Yang wrote:
> +	icsk->icsk_rto_min = usecs_to_jiffies(READ_ONCE(sock_net(sk)->
> +					      ipv4.sysctl_tcp_rto_min_us));

This is somewhat awkwardly broken into two lines.
Could you use a temp variable to save 

	rto_min_us = READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_rto_min_us);

?

Or just go over 80 chars, but preferably not. Chaining 3 calls in one
line is a bit Java-esque :S

With that feel free to add:

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

We also carry (a form of) this patch in Meta kernels.. :(

Re: [PATCH net-next 0/2] tcp: add sysctl_tcp_rto_min_us

[Tony Lu]

On Wed, May 29, 2024 at 04:49:39PM +0800, Jason Xing wrote:
> On Wed, May 29, 2024 at 3:21 PM Tony Lu <tonylu@linux.alibaba.com> wrote:
> >
> > On Tue, May 28, 2024 at 05:13:18PM +0000, Kevin Yang wrote:
> > > Adding a sysctl knob to allow user to specify a default
> > > rto_min at socket init time.
> > >
> > > After this patch series, the rto_min will has multiple sources:
> > > route option has the highest precedence, followed by the
> > > TCP_BPF_RTO_MIN socket option, followed by this new
> > > tcp_rto_min_us sysctl.
> >
> > For series:
> >
> > Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
> >
> > I strongly support those patches. For those who use cgroup v1 and want
> > to take effect with simple settings, sysctl is a good way.
> 
> It's not a good reason to use sysctl.
> 
> If you say so, why not introduce many sysctls to replace setsockopt
> operations. For example, introducing a new sysctl to disable delayed
> ack to improve the speed of transmission in some cases just for ease
> of use? No, it's not right, I believe.
> 

Hidden behind the words is that if I am a kernel engineer or SRE helping
users troubleshoot latency issues, and I need to tune tcp_rto_min, then
my only means of not intruding on the application are eBPF or the sysctl
mentioned here.

Comparing sysctl and eBPF, I prefer sysctl isolated by net namespace,
which can be modified and verified more directly and quickly. eBPF is
powerful, but it is not easy to write, debug and manage.

> >
> > And reducing it is helpful for latency-sensitive applications such as
> > Redis, net namespace level sysctl knob is enough.
> 
> Sure, these key parameters play a big role in the TCP stack.
> 
> >
> > >
> > > Kevin Yang (2):
> > >   tcp: derive delack_max with tcp_rto_min helper
> > >   tcp: add sysctl_tcp_rto_min_us
> > >
> > >  Documentation/networking/ip-sysctl.rst | 13 +++++++++++++
> > >  include/net/netns/ipv4.h               |  1 +
> > >  net/ipv4/sysctl_net_ipv4.c             |  8 ++++++++
> > >  net/ipv4/tcp.c                         |  3 ++-
> > >  net/ipv4/tcp_ipv4.c                    |  1 +
> > >  net/ipv4/tcp_output.c                  | 11 ++---------
> > >  6 files changed, 27 insertions(+), 10 deletions(-)
> > >
> > > --
> > > 2.45.1.288.g0e0cd299f1-goog
> > >
> >