Message-ID: <ca443d9a-3809-5c05-dac4-99f8ba313cb5@netronome.com>
Date:   Mon, 22 Oct 2018 11:00:27 +0100
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH bpf-next] selftests/bpf: enable (uncomment) all tests in
 test_libbpf.sh

2018-10-21 23:04 UTC+0200 ~ Jesper Dangaard Brouer <brouer@...hat.com>
> 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".

Alright, I can repost my second version that takes a test out of the
default target for building BPF programs, after the merge window.

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

This is already the case. What happens for this test is that we never
tell libbpf that this program is XDP, we just ask it to open the ELF
file and the whole time libbpf treats it as a program of type
BPF_PROG_TYPE_UNSPEC. So we can fix the BPF source (by adding a version)
or we can fix test_libbpf_open.c (to tell libbpf this is XDP), but I
don't believe there is anything to add to libbpf in that regard. I think
we could simply remove the test on test_xdp_meta.o from test_libbpf.h,
actually. What is you opinion?

Thanks,
Quentin

Powered by blists - more mailing lists