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

[Martin KaFai Lau <martin.lau@linux.dev>]

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, },
>   };
>