From: Shuah Khan <email@example.com>
To: Daniel Borkmann <firstname.lastname@example.org>,
Alexei Starovoitov <email@example.com>,
Edward Cree <firstname.lastname@example.org>
Cc: Shuah Khan <email@example.com>, Thomas Meyer <firstname.lastname@example.org>,
Shuah Khan <email@example.com>
Subject: Re: selftests/bpf doesn't compile
Date: Fri, 15 Sep 2017 16:41:15 -0600 [thread overview]
Message-ID: <firstname.lastname@example.org> (raw)
On 09/15/2017 12:48 PM, Daniel Borkmann wrote:
> On 09/15/2017 08:23 PM, Daniel Borkmann wrote:
>> On 09/15/2017 08:07 PM, Alexei Starovoitov wrote:
>>> On Fri, Sep 15, 2017 at 05:58:40PM +0100, Edward Cree wrote:
>>>> On 15/09/17 17:02, Alexei Starovoitov wrote:
>>>>> On Thu, Sep 14, 2017 at 09:33:48AM -0600, Shuah Khan wrote:
>>>>>> Is bpf test intended to be run in kselftest run? The clang dependency might
>>>>>> not be met on majority of the systems. Is this a hard dependency??
>>>>> It is a hard dependency and clang should be present on majority of the systems.
>>>> I think this is the wrong approach. Making kselftest hard-require clang doesn't
>>>> mean that the bpf tests will be run more often, it means that the rest of the
>>>> kselftests will be run less often. clang is quite big (when I tried to install
>>>> it on one of my test servers, I didn't have enough disk space & had to go on a
>>>> clear-out of unused packages), and most people aren't interested in the bpf
>>>> subsystem specifically; they would rather be able to skip those tests.
>>>> I feel that as long as they know they are skipping some tests (so e.g. they
>>>> won't consider it a sufficient test of a kselftest refactor), that's fine.
>>>> It's not even as though all of the bpf tests require clang; the (smaller) tests
>>>> written directly in raw eBPF instructions could still be run on such a system.
>>>> So I think we should attempt to run as much as possible but accept that clang
>>>> may not be available and have an option to skip some tests in that case.
>>> imo the value of selftests/bpf is twofold:
>>> 1. it helps bpf developers avoid regressions
>>> 2. as part of continuous integration it helps to catch bpf regressions
>>> that were somehow caused by changes in other parts of the kernel
>>> If a developer didn't bother to satisfy all bpf tests dependencies
>>> (which includes clang) and ran all tests before sending a patch,
>>> I don't want to see such patches. It just wastes maintainers time
>>> to review code and spot bugs that could have been caught by tests.
>>> Collectively we invested years of work into these tests and
>>> developers better take advantage of it by running all.
>>> If a CI server didn't satisfy all bpf test dependencies,
>>> I don't want such CI setup to be running and reporting results,
>>> since it will give false sense of test coverage.
>>> Test failures due to missing dependencies are hard failures.
>>> We cannot skip them.
> Btw, on that note, the folks from zero-day bot run the BPF kselftests
> for a while now just fine and they do run them together with clang,
> so they have the full, proper coverage how it should be. It's not
> how it used to be in the early days, you can just go and install
> llvm/clang package on all the major distros today and you get the
> bpf target by default enabled already.
>>> I'd like generic XDP tests to be added to selftests/bpf which
>>> would mean that the latest iproute2 will become a hard dependency
>>> and bpf developers and CI host owners would need to upgrade
>>> their iproute2.
>>> The tests either pass or fail. Skipping them due to missing
>>> dependencies is the same as fail and in that sense I don't want
>>> to change selftests/bpf/Makefile to make it skip clang.
>> I fully agree that for the BPF selftests it is very desirable
>> to not only test the verifier with couple of BPF insn snippets,
>> but to actually load and run programs that more closely resemble
>> real world programs. For more complex interactions these snippets
>> are just limited, think of tail calls, testing perf event output
>> helper, etc, which would all require to write these tests with
>> restricted C when we add them (unless we want to make writing
>> these tests a real pain ;) in which case no-one will bother to
>> write tests at all for them). Mid to long term I would definitely
>> like to see more programs in BPF selftests (e.g. moved over from
>> samples/bpf/) to increase the test coverage.
As I said in my earlier email:
Unless users choose to install clang, bpf will always fail run without
clang. So clang dependency is an issue for bpf test coverage in general.
That is your choice as to whether you want to increase the scope of
regression test coverage for bpf or not.
I fully understand you have weigh that against ease of writing tests.
We can leave things the way they are since:
- You can't force users to install clang and run bpf test. Users might
opt for letting bpf test fail due to unmet dependency.
- You have reasons to continue use clang and you have been using it for
I will try to see why make ksefltest fails on bpf even with my patch
series that addresses the source directory issue. All other tests build
and run. It is not an issue with bpf specifically, it is something that
has never been tested in this use-case.
next prev parent reply other threads:[~2017-09-15 22:41 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-08 11:19 [PATCH] selftests/bpf: Make bpf_util work on uniprocessor systems Thomas Meyer
2017-09-08 23:01 ` Alexei Starovoitov
2017-09-08 23:05 ` Daniel Borkmann
2017-09-14 15:01 ` Shuah Khan
2017-09-14 15:33 ` selftests/bpf doesn't compile Shuah Khan
2017-09-15 16:02 ` Alexei Starovoitov
2017-09-15 16:58 ` Edward Cree
2017-09-15 18:07 ` Alexei Starovoitov
2017-09-15 18:23 ` Daniel Borkmann
2017-09-15 18:48 ` Daniel Borkmann
2017-09-15 22:41 ` Shuah Khan [this message]
2017-09-18 13:31 ` Daniel Borkmann
2017-09-15 17:00 ` Shuah Khan
2017-09-15 17:44 ` Shuah Khan
2017-09-15 18:14 ` Alexei Starovoitov
2017-09-15 22:32 ` Shuah Khan
2019-01-04 17:16 ` Geert Uytterhoeven
2019-01-04 19:07 ` shuah
2017-09-19 14:45 ` [PATCH] selftests/bpf: Make bpf_util work on uniprocessor systems Shuah Khan
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).