Re: [PATCHv3 bpf-next 1/9] selftests/bpf: Move kfunc exports to bpf_testmod/bpf_testmod_kfunc.h

Andrii Nakryiko
2023-02-09T00:20:29Z

On Tue, Feb 7, 2023 at 6:28 AM David Vernet <void@manifault.com> wrote:
>
> On Fri, Feb 03, 2023 at 05:23:28PM +0100, Jiri Olsa wrote:
> > Move all kfunc exports into separate bpf_testmod_kfunc.h header file
> > and include it in tests that need it.
> >
> > We will move all test kfuncs into bpf_testmod in following change,
> > so it's convenient to have declarations in single place.
> >
> > The bpf_testmod_kfunc.h is included by both bpf_testmod and bpf
> > programs that use test kfuncs.
> >
> > As suggested by David, the bpf_testmod_kfunc.h includes vmlinux.h
> > and bpf/bpf_helpers.h for bpf programs build, so the declarations
> > have proper __ksym attribute and we can resolve all the structs.
> >
> > Note in kfunc_call_test_subprog.c we can no longer use the sk_state
> > define from bpf_tcp_helpers.h (because it clashed with vmlinux.h)
> > and we need to address __sk_common.skc_state field directly.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > .../bpf/bpf_testmod/bpf_testmod_kfunc.h | 41 +++++++++++++++++++
> > tools/testing/selftests/bpf/progs/cb_refs.c | 4 +-
> > .../selftests/bpf/progs/jit_probe_mem.c | 4 +-
> > .../bpf/progs/kfunc_call_destructive.c | 3 +-
> > .../selftests/bpf/progs/kfunc_call_fail.c | 9 +---
> > .../selftests/bpf/progs/kfunc_call_race.c | 3 +-
> > .../selftests/bpf/progs/kfunc_call_test.c | 17 +-------
> > .../bpf/progs/kfunc_call_test_subprog.c | 9 +---
> > tools/testing/selftests/bpf/progs/map_kptr.c | 6 +--
> > .../selftests/bpf/progs/map_kptr_fail.c | 5 +--
> > 10 files changed, 51 insertions(+), 50 deletions(-)
> > create mode 100644 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> >
> > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> > new file mode 100644
> > index 000000000000..86d94257716a
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _BPF_TESTMOD_KFUNC_H
> > +#define _BPF_TESTMOD_KFUNC_H
> > +
> > +#ifndef __KERNEL__
> > +#include <vmlinux.h>
> > +#include <bpf/bpf_helpers.h>
> > +#else
> > +#define __ksym
> > +#endif
> > +
> > +extern struct prog_test_ref_kfunc *
> > +bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr) __ksym;
> > +extern struct prog_test_ref_kfunc *
> > +bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
> > +extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> > +
> > +extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
> > +extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
> > +extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> > +extern int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> > +extern void bpf_kfunc_call_int_mem_release(int *p) __ksym;
> > +extern u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused) __ksym;
> > +
> > +extern void bpf_testmod_test_mod_kfunc(int i) __ksym;
> > +
> > +extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
> > + __u32 c, __u64 d) __ksym;
> > +extern int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
> > +extern struct sock *bpf_kfunc_call_test3(struct sock *sk) __ksym;
> > +extern long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
> > +
> > +extern void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb) __ksym;
> > +extern void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym;
> > +extern void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym;
> > +extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
> > +
> > +extern void bpf_kfunc_call_test_destructive(void) __ksym;
> > +
> > +#endif /* _BPF_TESTMOD_KFUNC_H */
> > diff --git a/tools/testing/selftests/bpf/progs/cb_refs.c b/tools/testing/selftests/bpf/progs/cb_refs.c
> > index 7653df1bc787..823901c1b839 100644
> > --- a/tools/testing/selftests/bpf/progs/cb_refs.c
> > +++ b/tools/testing/selftests/bpf/progs/cb_refs.c
> > @@ -2,6 +2,7 @@
> > #include <vmlinux.h>
> > #include <bpf/bpf_tracing.h>
> > #include <bpf/bpf_helpers.h>
> > +#include "bpf_testmod/bpf_testmod_kfunc.h"
>
> Feel free to ignore if you disagree, but here and elsewhere, should we
> do this:
>
> #include <bpf_testmod/bpf_testmod_kfunc.h>
>
> rather than using #include "bpf_testmod/bpf_testmod_kfunc.h". Doesn't
> matter much, but IMO it's just slightly more readable to use the <> to
> show that we're relying on -I rather than expecting
> bpf_testmod/bpf_testmod_kfunc.h to be found at a path relative to the
> progs. #include "bpf_misc.h" makes more sense because it really is
> located in the progs/ directory.

We do <> for headers that are expected to be installed in the system
(even if we cheat with -I sometimes). But in this case it's a local
header, so using "" makes more sense to me. But shouldn't it be
"../bpf_testmod/bpf_testmod_kfunc.h"?

>
> Either way:
>
> Acked-by: David Vernet <void@manifault.com>
>
> > …