Re: [PATCH v3 9/9] selftests/bpf/sockopt: Add io_uring support

[Breno Leitao <leitao@debian.org>]

On Mon, Aug 21, 2023 at 01:59:12PM -0700, Martin KaFai Lau wrote:
> On 8/17/23 7:55 AM, Breno Leitao wrote:
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 538df8fb8c42..4da04242b848 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -362,6 +362,7 @@ CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \
> >   $(OUTPUT)/test_l4lb_noinline.o: BPF_CFLAGS += -fno-inline
> >   $(OUTPUT)/test_xdp_noinline.o: BPF_CFLAGS += -fno-inline
> > +$(OUTPUT)/test_progs.o: CFLAGS += -I../../../include/
> 
> This is the tools/include? Is it really needed? iirc, some of the
> prog_tests/*.c has already been using files from tools/include.

You are right, we don't need it.

> >   $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h
> >   $(OUTPUT)/cgroup_getset_retval_hooks.o: cgroup_getset_retval_hooks.h
> > diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c
> > index 9e6a5e3ed4de..4693ad8bfe8f 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c
> > @@ -1,5 +1,6 @@
> >   // SPDX-License-Identifier: GPL-2.0
> >   #include <test_progs.h>
> > +#include <io_uring/mini_liburing.h>
> >   #include "cgroup_helpers.h"
> >   static char bpf_log_buf[4096];
> > @@ -38,6 +39,7 @@ static struct sockopt_test {
> >   	socklen_t			get_optlen_ret;
> >   	enum sockopt_test_error		error;
> > +	bool				io_uring_support;
> >   } tests[] = {
> >   	/* ==================== getsockopt ====================  */
> > @@ -53,6 +55,7 @@ static struct sockopt_test {
> >   		.attach_type = BPF_CGROUP_GETSOCKOPT,
> >   		.expected_attach_type = 0,
> >   		.error = DENY_LOAD,
> > +		.io_uring_support = true,
> 
> DENY_LOAD probably won't be an intersting test. The set/getsockopt won't be called.

Yea, I will remove all the DENY_LOAD and DENY_ATTACH tests.

> The existing test does not seem to have SOL_SOCKET for getsockopt also.

I am planning to move two tests to use SOL_SOCKET so we can also
exercise the io_uring tests. This is what I have in mind right now:

 * getsockopt: read ctx->optlen
 * getsockopt: support smaller ctx->optlen

> > -static int run_test(int cgroup_fd, struct sockopt_test *test)
> > +/* Core function that handles io_uring ring initialization,
> > + * sending SQE with sockopt command and waiting for the CQE.
> > + */
> > +static int uring_sockopt(int op, int fd, int level, int optname,
> > +			 const void *optval, socklen_t optlen)
> > +{
> > +	struct io_uring_cqe *cqe;
> > +	struct io_uring_sqe *sqe;
> > +	struct io_uring ring;
> > +	int err;
> > +
> > +	err = io_uring_queue_init(1, &ring, 0);
> > +	if (err) {
> > +		log_err("Failed to initialize io_uring ring");
> > +		return err;
> > +	}
> > +
> > +	sqe = io_uring_get_sqe(&ring);
> > +	if (!sqe) {
> > +		log_err("Failed to get an SQE");
> > +		return -1;
> 
> No need to io_uring_queue_exit() on the error path?

Good idea. updating it.

> > +	}
> > +
> > +	io_uring_prep_cmd(sqe, op, fd, level, optname, optval, optlen);
> > +
> > +	err = io_uring_submit(&ring);
> > +	if (err != 1) {
> > +		log_err("Failed to submit SQE");
> 
> Use ASSERT_* instead.
> 
> Regarding how to land this set,
> it will be useful to have the selftest running in the bpf CI. While there is
> iouring changes, some of the changes is in bpf and/or netdev also. eg. Patch
> 3 already has a conflict with the net-next and bpf-next tree because of a
> recent commit in socket.c on Aug 9.
> 
> May be Alexi and Daniel can advise how was similar patch managed before ?
> 
>