netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stanislav Fomichev <sdf@fomichev.me>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Edward Cree <ecree@solarflare.com>,
	Stanislav Fomichev <sdf@google.com>,
	netdev@vger.kernel.org, davem@davemloft.net, ast@kernel.org,
	daniel@iogearbox.net
Subject: Re: [PATCH bpf-next 1/2] selftests/bpf: skip verifier tests that depend on CONFIG_CGROUP_BPF
Date: Wed, 12 Dec 2018 14:32:01 -0800	[thread overview]
Message-ID: <20181212223201.GG9107@mini-arch.hsd1.ca.comcast.net> (raw)
In-Reply-To: <20181212220703.ubk6a6gmjr7no7wa@ast-mbp.dhcp.thefacebook.com>

On 12/12, Alexei Starovoitov wrote:
> On Wed, Dec 12, 2018 at 09:23:30PM +0000, Edward Cree wrote:
> > On 12/12/18 20:13, Alexei Starovoitov wrote:
> > > that won't work either.
> > > "bpf feature set" is a lot more than number of program
> > > and map types the kernel supports. There are all sorts of
> > > helper combinations, hooks, and verifier improvements.
> > > test_verifier.c must test all that.
> > > I don't think there is a way to make usptream test_verfier.c
> > > not to report failure on older kernels.
> > But it's not just older kernels; AIUI there are config options
> >  that also affect this.  Are you saying that test_verifier
> >  should only be expected to run / pass on allyesconfig kernels?
> 
> not allyesconfig, but allbpfyesconfig.
Yes, to reiterate, this patch series was mostly about running verifier
against the kernel where we don't have all bpf features enabled (to make
sure we also don't break them by having some (un)related inhouse patches).

> > I think that for the cases where we _can_ do it easily (which
> >  seems to be precisely things like prog_type which don't require
> >  any additional annotation of test cases) we should skip tests
> >  that aren't supported by the running kernel.
> 
> Submitters of bpf patches need to enable all bpf features in
> their kernels and make sure that all of the selftests/bpf
> are still passing.
> "but I don't use XDP today" or "LLVM is hard to install"
> is not an excuse.
> 
> Before I apply any patch I build and test manually.
> If anything breaks means folks didn't do enough testing
> and karma suffers.
Your use case would still work, you just make sure there are no skips.
Karma would still suffer if you run the tests and they fail, but for the
submitter they were skipped ;-)

To summarize, I like your idea about doing runtime tests and I think I
can make it work quite nicely without any config_disabled ugliness by
looking at the prog_type of each test.
I can send an RFC patch series out if there still a small chance you could
take it, but if you've already set you mind, I'd just keep them
internally. So let me know if you have a hard NACK on the runtime probing
approach or there is still some wiggle room.

  reply	other threads:[~2018-12-12 22:32 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-12 18:27 [PATCH bpf-next 1/2] selftests/bpf: skip verifier tests that depend on CONFIG_CGROUP_BPF Stanislav Fomichev
2018-12-12 18:27 ` [PATCH bpf-next 2/2] selftests/bpf: skip verifier tests that depend on CONFIG_BPF_EVENTS Stanislav Fomichev
2018-12-12 18:51 ` [PATCH bpf-next 1/2] selftests/bpf: skip verifier tests that depend on CONFIG_CGROUP_BPF Alexei Starovoitov
2018-12-12 18:59   ` Stanislav Fomichev
2018-12-12 19:04     ` Alexei Starovoitov
2018-12-12 19:24       ` Stanislav Fomichev
2018-12-12 19:54         ` Stanislav Fomichev
2018-12-12 20:13           ` Alexei Starovoitov
2018-12-12 21:23             ` Edward Cree
2018-12-12 22:07               ` Alexei Starovoitov
2018-12-12 22:32                 ` Stanislav Fomichev [this message]
2018-12-13  6:06                   ` Alexei Starovoitov
2018-12-13 11:52                     ` Daniel Borkmann
2018-12-13 12:18                       ` Quentin Monnet
2018-12-13 16:37                         ` Stanislav Fomichev
2018-12-13 17:02                           ` Quentin Monnet
2018-12-13 17:10                             ` Stanislav Fomichev
2018-12-14 11:43                               ` Quentin Monnet

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=20181212223201.GG9107@mini-arch.hsd1.ca.comcast.net \
    --to=sdf@fomichev.me \
    --cc=alexei.starovoitov@gmail.com \
    --cc=ast@kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=ecree@solarflare.com \
    --cc=netdev@vger.kernel.org \
    --cc=sdf@google.com \
    /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
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).