Re: [PATCH bpf v2 2/2] selftests/bpf: Test TCP_NODELAY in TCP hdr opt callbacks

[KaFai Wan <kafai.wan@linux.dev>]

On Thu, 2026-04-16 at 12:06 -0700, Martin KaFai Lau wrote:
> On Thu, Apr 16, 2026 at 07:23:08PM +0800, KaFai Wan wrote:
> > index 56685fc03c7e..2d738c0c4259 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/tcp_hdr_options.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/tcp_hdr_options.c
> > @@ -513,6 +513,59 @@ static void misc(void)
> >  	bpf_link__destroy(link);
> >  }
> >  
> > +static void hdr_sockopt(void)
> > +{
> > +	const char send_msg[] = "MISC!!!";
> > +	char recv_msg[sizeof(send_msg)];
> > +	const unsigned int nr_data = 2;
> > +	struct bpf_link *link;
> > +	struct sk_fds sk_fds;
> > +	int i, ret, true_val = 1;
> > +
> > +	lport_linum_map_fd = bpf_map__fd(misc_skel->maps.lport_linum_map);
> > +
> > +	link = bpf_program__attach_cgroup(misc_skel->progs.misc_hdr_sockopt, cg_fd);
> > +	if (!ASSERT_OK_PTR(link, "attach_cgroup(misc_hdr_sockopt)"))
> > +		return;
> > +
> > +	if (sk_fds_connect(&sk_fds, false)) {
> > +		bpf_link__destroy(link);
> > +		return;
> > +	}
> > +
> > +	ret = setsockopt(sk_fds.active_fd, SOL_TCP, TCP_NODELAY, &true_val, sizeof(true_val));
> > +	if (!ASSERT_OK(ret, "setsockopt(TCP_NODELAY) active"))
> > +		goto check_linum;
> > +
> > +	ret = setsockopt(sk_fds.passive_fd, SOL_TCP, TCP_NODELAY, &true_val, sizeof(true_val));
> 
> Why are these two setsockopt(TCP_NODELAY) calls needed?
> 
> Instead of creating a new "void hdr_sockopt(void)", can the test be done in the
> existing "void misc(void)" by doing bpf_setsockopt(TCP_NODELAY) in the
> misc_estab() bpf prog?

Oh, I see. I meant to test on both active and passive side. We can only test on active side in the
existing "void misc(void)".
> 
> The PASSIVE_ESTABLISHED_CB can do the bpf_setsockopt(TCP_NODELAY, 0)
> if it wants to keep the same expectation on Nagle. The
> BPF_SOCK_OPS_HDR_OPT_LEN_CB and BPF_SOCK_OPS_WRITE_HDR_OPT_CB
> can do bpf_setsockopt(TCP_NODELAY, 1) to test recursion and
> the error return value.
> 
> >  void test_tcp_hdr_options(void)
> > diff --git a/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c
> > b/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c
> > index d487153a839d..a8cf7c4e7ed2 100644
> > --- a/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c
> > +++ b/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c
> > @@ -28,6 +28,12 @@ unsigned int nr_data = 0;
> >  unsigned int nr_syn = 0;
> >  unsigned int nr_fin = 0;
> >  unsigned int nr_hwtstamp = 0;
> > +unsigned int nr_hdr_sockopt_estab = 0;
> > +unsigned int nr_hdr_sockopt_estab_err = 0;
> > +unsigned int nr_hdr_sockopt_len = 0;
> > +unsigned int nr_hdr_sockopt_len_err = 0;
> > +unsigned int nr_hdr_sockopt_write = 0;
> > +unsigned int nr_hdr_sockopt_write_err = 0;
> 
> nr_hdr_sockopt_estab, nr_hdr_sockopt_len, and nr_hdr_sockopt_write
> are unnecessary. These tests have already been covered in some ways.

yes, they are unnecessary in existing misc_estab()
> 
> Mostly a nit. The new counters are used in both connections. Note the
> existing nr_xxx is exclusively used in either active or passive,
> so there is no parallel counting in practice.
> 
> Instead of counting, just use a bool nodelay_est_ok,
> nodelay_hdr_len_err, nodelay_write_err and assert them
> to be true in userspace.

indeed. will fix these in next version.

-- 
Thanks,
KaFai