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

[Martin KaFai Lau <martin.lau@xxxxxxxxx>]

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.

>   
>   $(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.

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

> -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?


> +	}
> +
> +	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 ?