On Wed, Feb 19, 2025 at 10:01 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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, },
> > };
> >
>
