From: Daniel Borkmann <email@example.com> To: Shuah Khan <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>, email@example.com, firstname.lastname@example.org, Networking <email@example.com> Subject: Re: selftests/bpf doesn't compile Date: Mon, 18 Sep 2017 15:31:32 +0200 [thread overview] Message-ID: <59BFCAB4.firstname.lastname@example.org> (raw) In-Reply-To: <email@example.com> On 09/16/2017 12:41 AM, Shuah Khan wrote: > 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. >>> >>> +1 >>> >>>> 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. >>> >>> +1 >> >> 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 > a longtime. I'm definitely for leaving it as it currently is and having clang as hard dependency; there will be more BPF selftests over time that will require to compile BPF progs (through clang's BPF backend) written in restricted C, so just skipping these tests would give a false sense of coverage. clang is pretty much needed anyway for writing more complex programs, thus leaving requirements the way they are is the much better option. > 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. Great, thanks!
next prev parent reply other threads:[~2017-09-18 13:31 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 2017-09-18 13:31 ` Daniel Borkmann [this message] 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
Reply instructions: 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: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=59BFCAB4.firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: selftests/bpf doesn'\''t compile' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
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).