Date:   Sun, 21 Oct 2018 23:04:38 +0200
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Quentin Monnet <quentin.monnet@...ronome.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        oss-drivers@...ronome.com, brouer@...hat.com
Subject: Re: [PATCH bpf-next] selftests/bpf: enable (uncomment) all tests in
 test_libbpf.sh

On Sun, 21 Oct 2018 16:37:08 +0100
Quentin Monnet <quentin.monnet@...ronome.com> wrote:

> 2018-10-21 11:57 UTC+0200 ~ Jesper Dangaard Brouer <brouer@...hat.com>
> > On Sat, 20 Oct 2018 23:00:24 +0100
> > Quentin Monnet <quentin.monnet@...ronome.com> wrote:
> >   
> 
> [...]
> 
> >> --- a/tools/testing/selftests/bpf/test_libbpf.sh
> >> +++ b/tools/testing/selftests/bpf/test_libbpf.sh
> >> @@ -33,17 +33,11 @@ trap exit_handler 0 2 3 6 9
> >>   
> >>   libbpf_open_file test_l4lb.o
> >>   
> >> -# TODO: fix libbpf to load noinline functions
> >> -# [warning] libbpf: incorrect bpf_call opcode
> >> -#libbpf_open_file test_l4lb_noinline.o
> >> +libbpf_open_file test_l4lb_noinline.o
> >>   
> >> -# TODO: fix test_xdp_meta.c to load with libbpf
> >> -# [warning] libbpf: test_xdp_meta.o doesn't provide kernel version
> >> -#libbpf_open_file test_xdp_meta.o
> >> +libbpf_open_file test_xdp_meta.o
> >>   
> >> -# TODO: fix libbpf to handle .eh_frame
> >> -# [warning] libbpf: relocation failed: no section(10)
> >> -#libbpf_open_file ../../../../samples/bpf/tracex3_kern.o
> >> +libbpf_open_file ../../../../samples/bpf/tracex3_kern.o  
> > 
> > I don't like the ../../../../samples/bpf/ reference (even-through I
> > added this TODO), as the kselftests AFAIK support installing the
> > selftests and then this tests will fail.
> > Maybe we can find another example kern.o file?
> > (which isn't compiled with -target bpf)  
> 
> Hi Jesper, yeah maybe making the test rely on something from samples/bpf
> instead of just the selftests/bpf directory is not a good idea. But
> there is no program compiled without the "-target-bpf" in that
> directory. What we could do is explicitly compile one without the flag
> in the Makefile, as in the patch below, but I am not sure that doing so
> is acceptable?

I think it makes sense to have a test program compiled without the
"-target-bpf", as that will happen for users.  And I guess we can add
some more specific test that are related to "-target-bpf".

> Or should tests for libbpf have a directory of their own,
> with another Makefile?

Hmm, I'm not sure about that idea.

I did plan by naming the test "libbpf_open_file", what we add more
libbpf_ prefixed tests to the test_libbpf.sh script, which should
cover more aspects of the _base_ libbpf functionality.

> Another question regarding the test with test_xdp_meta.o: does the fix I
> suggested (setting a version in the .C file) makes sense, or did you
> leave this test for testing someday that libbpf would be able to open
> even programs that do not set a version (in which case this is still not
> the case if program type is not provided, and in fact my fix ruins
> everything? :s).

Well, yes.  I was hinting if we should relax the version requirement
for e.g. XDP BPF progs.

> ---
>  tools/testing/selftests/bpf/Makefile        | 10 ++++++++++
>  tools/testing/selftests/bpf/test_libbpf.sh  | 14 +++++---------
>  tools/testing/selftests/bpf/test_xdp_meta.c |  2 ++
>  3 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index e39dfb4e7970..ecd79b7fb107 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -135,6 +135,16 @@ endif
>  endif
>  endif
>  
> +# Have one program compiled without "-target bpf" to test whether libbpf loads
> +# it successfully
> +$(OUTPUT)/test_xdp.o: test_xdp.c
> +	$(CLANG) $(CLANG_FLAGS) \
> +		-O2 -emit-llvm -c $< -o - | \
> +	$(LLC) -march=bpf -mcpu=$(CPU) $(LLC_FLAGS) -filetype=obj -o $@
> +ifeq ($(DWARF2BTF),y)
> +	$(BTF_PAHOLE) -J $@
> +endif
> +
>  $(OUTPUT)/%.o: %.c
>  	$(CLANG) $(CLANG_FLAGS) \
>  		 -O2 -target bpf -emit-llvm -c $< -o - |      \
> diff --git a/tools/testing/selftests/bpf/test_libbpf.sh b/tools/testing/selftests/bpf/test_libbpf.sh
> index 156d89f1edcc..b45962a44243 100755
> --- a/tools/testing/selftests/bpf/test_libbpf.sh
> +++ b/tools/testing/selftests/bpf/test_libbpf.sh
> @@ -33,17 +33,13 @@ trap exit_handler 0 2 3 6 9
>  
>  libbpf_open_file test_l4lb.o
>  
> -# TODO: fix libbpf to load noinline functions
> -# [warning] libbpf: incorrect bpf_call opcode
> -#libbpf_open_file test_l4lb_noinline.o
> +# Load a program with BPF-to-BPF calls
> +libbpf_open_file test_l4lb_noinline.o
>  
> -# TODO: fix test_xdp_meta.c to load with libbpf
> -# [warning] libbpf: test_xdp_meta.o doesn't provide kernel version
> -#libbpf_open_file test_xdp_meta.o
> +libbpf_open_file test_xdp_meta.o
>  
> -# TODO: fix libbpf to handle .eh_frame
> -# [warning] libbpf: relocation failed: no section(10)
> -#libbpf_open_file ../../../../samples/bpf/tracex3_kern.o
> +# Load a program compiled without the "-target bpf" flag
> +libbpf_open_file test_xdp.o
>  
>  # Success
>  exit 0
> diff --git a/tools/testing/selftests/bpf/test_xdp_meta.c b/tools/testing/selftests/bpf/test_xdp_meta.c
> index 8d0182650653..2f42de66e2bb 100644
> --- a/tools/testing/selftests/bpf/test_xdp_meta.c
> +++ b/tools/testing/selftests/bpf/test_xdp_meta.c
> @@ -8,6 +8,8 @@
>  #define round_up(x, y) ((((x) - 1) | __round_mask(x, y)) + 1)
>  #define ctx_ptr(ctx, mem) (void *)(unsigned long)ctx->mem
>  
> +int _version SEC("version") = 1;
> +
>  SEC("t")
>  int ing_cls(struct __sk_buff *ctx)
>  {



-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Powered by blists - more mailing lists