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