[PATCH bpf-next v2 0/3] bpf: support setting max RTO for bpf_setsockopt

[PATCH bpf-next v2 0/3] bpf: support setting max RTO for bpf_setsockopt

[Jason Xing]

Support max RTO set by BPF program calling bpf_setsockopt().
Add corresponding selftests.

Jason Xing (3):
  tcp: add TCP_RTO_MAX_MIN_SEC definition
  bpf: support TCP_RTO_MAX_MS for bpf_setsockopt
  selftests/bpf: add rto max for bpf_setsockopt test

 include/net/tcp.h                                   | 1 +
 net/core/filter.c                                   | 1 +
 net/ipv4/sysctl_net_ipv4.c                          | 3 ++-
 net/ipv4/tcp.c                                      | 3 ++-
 tools/include/uapi/linux/tcp.h                      | 1 +
 tools/testing/selftests/bpf/progs/bpf_tracing_net.h | 1 +
 tools/testing/selftests/bpf/progs/setget_sockopt.c  | 1 +
 7 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.43.5

[PATCH bpf-next v2 1/3] tcp: add TCP_RTO_MAX_MIN_SEC definition

[Jason Xing]

Add minimum value definition as the lower bound of RTO MAX
set by users. No functional changes here.

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 include/net/tcp.h          | 1 +
 net/ipv4/sysctl_net_ipv4.c | 3 ++-
 net/ipv4/tcp.c             | 3 ++-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 7fd2d7fa4532..b6bedbe68636 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -143,6 +143,7 @@ static_assert((1 << ATO_BITS) > TCP_DELACK_MAX);
 #define TCP_DELACK_MIN	4U
 #define TCP_ATO_MIN	4U
 #endif
+#define TCP_RTO_MAX_MIN_SEC 1
 #define TCP_RTO_MAX_SEC 120
 #define TCP_RTO_MAX	((unsigned)(TCP_RTO_MAX_SEC * HZ))
 #define TCP_RTO_MIN	((unsigned)(HZ / 5))
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 3a43010d726f..53942c225e0b 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -28,6 +28,7 @@ static int tcp_adv_win_scale_max = 31;
 static int tcp_app_win_max = 31;
 static int tcp_min_snd_mss_min = TCP_MIN_SND_MSS;
 static int tcp_min_snd_mss_max = 65535;
+static int tcp_rto_max_min = TCP_RTO_MAX_MIN_SEC * MSEC_PER_SEC;
 static int tcp_rto_max_max = TCP_RTO_MAX_SEC * MSEC_PER_SEC;
 static int ip_privileged_port_min;
 static int ip_privileged_port_max = 65535;
@@ -1590,7 +1591,7 @@ static struct ctl_table ipv4_net_table[] = {
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= SYSCTL_ONE_THOUSAND,
+		.extra1		= &tcp_rto_max_min,
 		.extra2		= &tcp_rto_max_max,
 	},
 };
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 992d5c9b2487..2373ab1a1d47 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3812,7 +3812,8 @@ int do_tcp_setsockopt(struct sock *sk, int level, int optname,
 					   TCP_RTO_MAX / HZ));
 		return 0;
 	case TCP_RTO_MAX_MS:
-		if (val < MSEC_PER_SEC || val > TCP_RTO_MAX_SEC * MSEC_PER_SEC)
+		if (val < TCP_RTO_MAX_MIN_SEC * MSEC_PER_SEC ||
+		    val > TCP_RTO_MAX_SEC * MSEC_PER_SEC)
 			return -EINVAL;
 		WRITE_ONCE(inet_csk(sk)->icsk_rto_max, msecs_to_jiffies(val));
 		return 0;
-- 
2.43.5

Re: [PATCH bpf-next v2 1/3] tcp: add TCP_RTO_MAX_MIN_SEC definition

[Martin KaFai Lau]

On 2/16/25 7:42 PM, Jason Xing wrote:
> Add minimum value definition as the lower bound of RTO MAX
> set by users. No functional changes here.

If it is no-op, why it is needed? The commit message didn't explain it either.
I also cannot guess how patch 2 depends on patch 1.

pw-bot: cr

Re: [PATCH bpf-next v2 1/3] tcp: add TCP_RTO_MAX_MIN_SEC definition

[Jason Xing]

On Wed, Feb 19, 2025 at 7:38 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/16/25 7:42 PM, Jason Xing wrote:
> > Add minimum value definition as the lower bound of RTO MAX
> > set by users. No functional changes here.
>
> If it is no-op, why it is needed? The commit message didn't explain it either.
> I also cannot guess how patch 2 depends on patch 1.

It's more of a cleanup patch :)

Thanks,
Jason

Re: [PATCH bpf-next v2 1/3] tcp: add TCP_RTO_MAX_MIN_SEC definition

[Jakub Kicinski]

On Tue, 18 Feb 2025 15:38:17 -0800 Martin KaFai Lau wrote:
> On 2/16/25 7:42 PM, Jason Xing wrote:
> > Add minimum value definition as the lower bound of RTO MAX
> > set by users. No functional changes here.  
> 
> If it is no-op, why it is needed? The commit message didn't explain it either.
> I also cannot guess how patch 2 depends on patch 1.

FWIW this patch also gave me pause when looking at v1.
I don't think this define makes the code any easier to follow.

Re: [PATCH bpf-next v2 1/3] tcp: add TCP_RTO_MAX_MIN_SEC definition

[Jason Xing]

On Wed, Feb 19, 2025 at 9:47 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 18 Feb 2025 15:38:17 -0800 Martin KaFai Lau wrote:
> > On 2/16/25 7:42 PM, Jason Xing wrote:
> > > Add minimum value definition as the lower bound of RTO MAX
> > > set by users. No functional changes here.
> >
> > If it is no-op, why it is needed? The commit message didn't explain it either.
> > I also cannot guess how patch 2 depends on patch 1.
>
> FWIW this patch also gave me pause when looking at v1.
> I don't think this define makes the code any easier to follow.

Okay, I will drop this in the next re-spin.

Thanks,
Jason

[PATCH bpf-next v2 2/3] bpf: support TCP_RTO_MAX_MS for bpf_setsockopt

[Jason Xing]

Some applications don't want to wait for too long because the
time of retransmission increases exponentially and can reach more
than 10 seconds, for example. Eric implements the core logic
on supporting rto max feature in the stack previously. Based on that,
we can support it for BPF use.

This patch reuses the same logic of TCP_RTO_MAX_MS in do_tcp_setsockopt()
and do_tcp_getsockopt(). BPF program can call bpf_{set/get}sockopt()
to set/get the maximum value of RTO.

It would be good if a BPF program sets max value of RTO before
transmission as we can see in the later patch (selftests).

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 net/core/filter.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index 2ec162dd83c4..ffec7b4357f9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5382,6 +5382,7 @@ static int sol_tcp_sockopt(struct sock *sk, int optname,
 	case TCP_USER_TIMEOUT:
 	case TCP_NOTSENT_LOWAT:
 	case TCP_SAVE_SYN:
+	case TCP_RTO_MAX_MS:
 		if (*optlen != sizeof(int))
 			return -EINVAL;
 		break;
-- 
2.43.5

Re: [PATCH bpf-next v2 2/3] bpf: support TCP_RTO_MAX_MS for bpf_setsockopt

[Kuniyuki Iwashima]

From: Jason Xing <kerneljasonxing@gmail.com>
Date: Mon, 17 Feb 2025 11:42:44 +0800
> Some applications don't want to wait for too long because the
> time of retransmission increases exponentially and can reach more
> than 10 seconds, for example. Eric implements the core logic
> on supporting rto max feature in the stack previously. Based on that,
> we can support it for BPF use.
> 
> This patch reuses the same logic of TCP_RTO_MAX_MS in do_tcp_setsockopt()
> and do_tcp_getsockopt(). BPF program can call bpf_{set/get}sockopt()
> to set/get the maximum value of RTO.
> 
> It would be good if a BPF program sets max value of RTO before
> transmission as we can see in the later patch (selftests).
> 
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

[PATCH bpf-next v2 3/3] selftests/bpf: add rto max for bpf_setsockopt test

[Jason Xing]

Add TCP_RTO_MAX_MS selftests for active and passive flows
in various bpf callbacks. Even though the TCP_RTO_MAX_MS
can be used in established phase, we highly discourage
to do so because it may trigger unexpected behaviour.
On the contrary, it's highly recommended that the maximum
value of RTO is set before first time of transmission, such
as BPF_SOCK_OPS_{PASSIVE|ACTIVE}_ESTABLISHED_CB,

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 tools/include/uapi/linux/tcp.h                      | 1 +
 tools/testing/selftests/bpf/progs/bpf_tracing_net.h | 1 +
 tools/testing/selftests/bpf/progs/setget_sockopt.c  | 1 +
 3 files changed, 3 insertions(+)

diff --git a/tools/include/uapi/linux/tcp.h b/tools/include/uapi/linux/tcp.h
index 13ceeb395eb8..7989e3f34a58 100644
--- a/tools/include/uapi/linux/tcp.h
+++ b/tools/include/uapi/linux/tcp.h
@@ -128,6 +128,7 @@ enum {
 #define TCP_CM_INQ		TCP_INQ
 
 #define TCP_TX_DELAY		37	/* delay outgoing packets by XX usec */
+#define TCP_RTO_MAX_MS		44	/* max rto time in ms */
 
 
 #define TCP_REPAIR_ON		1
diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
index 59843b430f76..eb6ed1b7b2ef 100644
--- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
+++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
@@ -49,6 +49,7 @@
 #define TCP_SAVED_SYN		28
 #define TCP_CA_NAME_MAX		16
 #define TCP_NAGLE_OFF		1
+#define TCP_RTO_MAX_MS		44
 
 #define TCP_ECN_OK              1
 #define TCP_ECN_QUEUE_CWR       2
diff --git a/tools/testing/selftests/bpf/progs/setget_sockopt.c b/tools/testing/selftests/bpf/progs/setget_sockopt.c
index 6dd4318debbf..106fe430f41b 100644
--- a/tools/testing/selftests/bpf/progs/setget_sockopt.c
+++ b/tools/testing/selftests/bpf/progs/setget_sockopt.c
@@ -61,6 +61,7 @@ static const struct sockopt_test sol_tcp_tests[] = {
 	{ .opt = TCP_NOTSENT_LOWAT, .new = 1314, .expected = 1314, },
 	{ .opt = TCP_BPF_SOCK_OPS_CB_FLAGS, .new = BPF_SOCK_OPS_ALL_CB_FLAGS,
 	  .expected = BPF_SOCK_OPS_ALL_CB_FLAGS, },
+	{ .opt = TCP_RTO_MAX_MS, .new = 2000, .expected = 2000, },
 	{ .opt = 0, },
 };
 
-- 
2.43.5

Re: [PATCH bpf-next v2 3/3] selftests/bpf: add rto max for bpf_setsockopt test

[Martin KaFai Lau]

On 2/16/25 7:42 PM, Jason Xing wrote:
> Add TCP_RTO_MAX_MS selftests for active and passive flows
> in various bpf callbacks. Even though the TCP_RTO_MAX_MS
> can be used in established phase, we highly discourage
> to do so because it may trigger unexpected behaviour.
> On the contrary, it's highly recommended that the maximum
> value of RTO is set before first time of transmission, such
> as BPF_SOCK_OPS_{PASSIVE|ACTIVE}_ESTABLISHED_CB,

s/,/./

What unexpected behavior when setting in BPF after the established state?

Setting it after the established state or not is not specific to BPF. syscall 
can choose to do it after the connection established also. The above makes it 
unclear what unexpected behavior that the BPF prog will cause if TCP_RTO_MAX_MS 
is used in BPF instead of syscall.

If there is subtle difference between calling TCP_RTO_MAX_MS from bpf and from 
syscall, please write it clearly what are the unexpected behaviors when calling 
in BPF after the established states.

Otherwise, the commit message can be just this:

Test the TCP_RTO_MAX_MS optname in the existing setget_sockopt test.

> 
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> ---
>   tools/include/uapi/linux/tcp.h                      | 1 +
>   tools/testing/selftests/bpf/progs/bpf_tracing_net.h | 1 +
>   tools/testing/selftests/bpf/progs/setget_sockopt.c  | 1 +
>   3 files changed, 3 insertions(+)
> 
> diff --git a/tools/include/uapi/linux/tcp.h b/tools/include/uapi/linux/tcp.h
> index 13ceeb395eb8..7989e3f34a58 100644
> --- a/tools/include/uapi/linux/tcp.h
> +++ b/tools/include/uapi/linux/tcp.h
> @@ -128,6 +128,7 @@ enum {
>   #define TCP_CM_INQ		TCP_INQ
>   
>   #define TCP_TX_DELAY		37	/* delay outgoing packets by XX usec */
> +#define TCP_RTO_MAX_MS		44	/* max rto time in ms */

Have you checked if this change is really needed?

>   
>   
>   #define TCP_REPAIR_ON		1
> diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> index 59843b430f76..eb6ed1b7b2ef 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> +++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> @@ -49,6 +49,7 @@
>   #define TCP_SAVED_SYN		28
>   #define TCP_CA_NAME_MAX		16
>   #define TCP_NAGLE_OFF		1
> +#define TCP_RTO_MAX_MS		44
>   
>   #define TCP_ECN_OK              1
>   #define TCP_ECN_QUEUE_CWR       2
> diff --git a/tools/testing/selftests/bpf/progs/setget_sockopt.c b/tools/testing/selftests/bpf/progs/setget_sockopt.c
> index 6dd4318debbf..106fe430f41b 100644
> --- a/tools/testing/selftests/bpf/progs/setget_sockopt.c
> +++ b/tools/testing/selftests/bpf/progs/setget_sockopt.c
> @@ -61,6 +61,7 @@ static const struct sockopt_test sol_tcp_tests[] = {
>   	{ .opt = TCP_NOTSENT_LOWAT, .new = 1314, .expected = 1314, },
>   	{ .opt = TCP_BPF_SOCK_OPS_CB_FLAGS, .new = BPF_SOCK_OPS_ALL_CB_FLAGS,
>   	  .expected = BPF_SOCK_OPS_ALL_CB_FLAGS, },
> +	{ .opt = TCP_RTO_MAX_MS, .new = 2000, .expected = 2000, },
>   	{ .opt = 0, },
>   };
>   

Re: [PATCH bpf-next v2 3/3] selftests/bpf: add rto max for bpf_setsockopt test

[Jason Xing]

On Wed, Feb 19, 2025 at 10:01 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/16/25 7:42 PM, Jason Xing wrote:
> > Add TCP_RTO_MAX_MS selftests for active and passive flows
> > in various bpf callbacks. Even though the TCP_RTO_MAX_MS
> > can be used in established phase, we highly discourage
> > to do so because it may trigger unexpected behaviour.
> > On the contrary, it's highly recommended that the maximum
> > value of RTO is set before first time of transmission, such
> > as BPF_SOCK_OPS_{PASSIVE|ACTIVE}_ESTABLISHED_CB,
>
> s/,/./
>
> What unexpected behavior when setting in BPF after the established state?
>
> Setting it after the established state or not is not specific to BPF. syscall
> can choose to do it after the connection established also. The above makes it
> unclear what unexpected behavior that the BPF prog will cause if TCP_RTO_MAX_MS
> is used in BPF instead of syscall.
>
> If there is subtle difference between calling TCP_RTO_MAX_MS from bpf and from
> syscall, please write it clearly what are the unexpected behaviors when calling
> in BPF after the established states.

I don't think there is any difference between them. For both of them,
It would be better to set before transmission.

>
> Otherwise, the commit message can be just this:
>
> Test the TCP_RTO_MAX_MS optname in the existing setget_sockopt test.

Got it. Will use this instead.

>
> >
> > Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> > ---
> >   tools/include/uapi/linux/tcp.h                      | 1 +
> >   tools/testing/selftests/bpf/progs/bpf_tracing_net.h | 1 +
> >   tools/testing/selftests/bpf/progs/setget_sockopt.c  | 1 +
> >   3 files changed, 3 insertions(+)
> >
> > diff --git a/tools/include/uapi/linux/tcp.h b/tools/include/uapi/linux/tcp.h
> > index 13ceeb395eb8..7989e3f34a58 100644
> > --- a/tools/include/uapi/linux/tcp.h
> > +++ b/tools/include/uapi/linux/tcp.h
> > @@ -128,6 +128,7 @@ enum {
> >   #define TCP_CM_INQ          TCP_INQ
> >
> >   #define TCP_TX_DELAY                37      /* delay outgoing packets by XX usec */
> > +#define TCP_RTO_MAX_MS               44      /* max rto time in ms */
>
> Have you checked if this change is really needed?

I thought we needed to sync it from include/uapi/linux/tcp.h. Will test it then.

Thanks,
Jason

>
> >
> >
> >   #define TCP_REPAIR_ON               1
> > diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> > index 59843b430f76..eb6ed1b7b2ef 100644
> > --- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> > +++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> > @@ -49,6 +49,7 @@
> >   #define TCP_SAVED_SYN               28
> >   #define TCP_CA_NAME_MAX             16
> >   #define TCP_NAGLE_OFF               1
> > +#define TCP_RTO_MAX_MS               44
> >
> >   #define TCP_ECN_OK              1
> >   #define TCP_ECN_QUEUE_CWR       2
> > diff --git a/tools/testing/selftests/bpf/progs/setget_sockopt.c b/tools/testing/selftests/bpf/progs/setget_sockopt.c
> > index 6dd4318debbf..106fe430f41b 100644
> > --- a/tools/testing/selftests/bpf/progs/setget_sockopt.c
> > +++ b/tools/testing/selftests/bpf/progs/setget_sockopt.c
> > @@ -61,6 +61,7 @@ static const struct sockopt_test sol_tcp_tests[] = {
> >       { .opt = TCP_NOTSENT_LOWAT, .new = 1314, .expected = 1314, },
> >       { .opt = TCP_BPF_SOCK_OPS_CB_FLAGS, .new = BPF_SOCK_OPS_ALL_CB_FLAGS,
> >         .expected = BPF_SOCK_OPS_ALL_CB_FLAGS, },
> > +     { .opt = TCP_RTO_MAX_MS, .new = 2000, .expected = 2000, },
> >       { .opt = 0, },
> >   };
> >
>