diff mbox series

[SRU,G,H,2/3] UBUNTU: SAUCE: selftests: Skip BPF seftests by default

Message ID 20201214220040.10644-3-kamal@canonical.com
State New
Headers show
Series disable building bpf selftests | expand

Commit Message

Kamal Mostafa Dec. 14, 2020, 10 p.m. UTC 
From: Mark Brown <broonie@kernel.org>

BugLink: https://bugs.launchpad.net/bugs/1908144

The BPF selftests have build time dependencies on cutting edge versions
of tools in the BPF ecosystem including LLVM which are more involved
to satisfy than more typical requirements like installing a package from
your distribution.  This causes issues for users looking at kselftest in
as a whole who find that a default build of kselftest fails and that
resolving this is time consuming and adds administrative overhead.  The
fast pace of BPF development and the need for a full BPF stack to do
substantial development or validation work on the code mean that people
working directly on it don't see a reasonable way to keep supporting
older environments without causing problems with the usability of the
BPF tests in BPF development so these requirements are unlikely to be
relaxed in the immediate future.

There is already support for skipping targets so in order to reduce the
barrier to entry for people interested in kselftest as a whole let's use
that to skip the BPF tests by default when people work with the top
level kselftest build system.  Users can still build the BPF selftests
as part of the wider kselftest build by specifying SKIP_TARGETS,
including setting an empty SKIP_TARGETS to build everything.  They can
also continue to build the BPF selftests individually in cases where
they are specifically focused on BPF.

This isn't ideal since it means people will need to take special steps
to build the BPF tests but the dependencies mean that realistically this
is already the case to some extent and it makes it easier for people to
pick up and work with the other selftests which is hopefully a net win.

Signed-off-by: Mark Brown <broonie@kernel.org>
Reference: https://lore.kernel.org/bpf/20201210185233.28091-1-broonie@kernel.org/
Signed-off-by: Kamal Mostafa <kamal@canonical.com>
---
 tools/testing/selftests/Makefile | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Seth Forshee Dec. 15, 2020, 1:52 p.m. UTC | #1 
On Mon, Dec 14, 2020 at 02:00:39PM -0800, Kamal Mostafa wrote:
> From: Mark Brown <broonie@kernel.org>
> 
> BugLink: https://bugs.launchpad.net/bugs/1908144
> 
> The BPF selftests have build time dependencies on cutting edge versions
> of tools in the BPF ecosystem including LLVM which are more involved
> to satisfy than more typical requirements like installing a package from
> your distribution.  This causes issues for users looking at kselftest in
> as a whole who find that a default build of kselftest fails and that
> resolving this is time consuming and adds administrative overhead.  The
> fast pace of BPF development and the need for a full BPF stack to do
> substantial development or validation work on the code mean that people
> working directly on it don't see a reasonable way to keep supporting
> older environments without causing problems with the usability of the
> BPF tests in BPF development so these requirements are unlikely to be
> relaxed in the immediate future.
> 
> There is already support for skipping targets so in order to reduce the
> barrier to entry for people interested in kselftest as a whole let's use
> that to skip the BPF tests by default when people work with the top
> level kselftest build system.  Users can still build the BPF selftests
> as part of the wider kselftest build by specifying SKIP_TARGETS,
> including setting an empty SKIP_TARGETS to build everything.  They can
> also continue to build the BPF selftests individually in cases where
> they are specifically focused on BPF.
> 
> This isn't ideal since it means people will need to take special steps
> to build the BPF tests but the dependencies mean that realistically this
> is already the case to some extent and it makes it easier for people to
> pick up and work with the other selftests which is hopefully a net win.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Reference: https://lore.kernel.org/bpf/20201210185233.28091-1-broonie@kernel.org/
> Signed-off-by: Kamal Mostafa <kamal@canonical.com>

In our autotests we build the bpf selftests prior to running the net
selftests, as some tests there require a module from the bpf selftests.
With this patch it looks like we will need to change that to override
SKIP_TARGETS so that bpf will not be filtered out. Is that correct?

Thanks,
Seth

> ---
>  tools/testing/selftests/Makefile | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index 1195bd85af38..195177ba44ab 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -71,8 +71,10 @@ TARGETS += zram
>  TARGETS_HOTPLUG = cpu-hotplug
>  TARGETS_HOTPLUG += memory-hotplug
>  
> -# User can optionally provide a TARGETS skiplist.
> -SKIP_TARGETS ?=
> +# User can optionally provide a TARGETS skiplist.  By default we skip
> +# BPF since it has cutting edge build time dependencies which require
> +# more effort to install.
> +SKIP_TARGETS ?= bpf
>  ifneq ($(SKIP_TARGETS),)
>  	TMP := $(filter-out $(SKIP_TARGETS), $(TARGETS))
>  	override TARGETS := $(TMP)
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Kamal Mostafa Dec. 15, 2020, 5:51 p.m. UTC | #2 
On Tue, Dec 15, 2020 at 07:52:10AM -0600, Seth Forshee wrote:
> On Mon, Dec 14, 2020 at 02:00:39PM -0800, Kamal Mostafa wrote:
> > From: Mark Brown <broonie@kernel.org>
> > 
> > BugLink: https://bugs.launchpad.net/bugs/1908144
> > 
> > The BPF selftests have build time dependencies on cutting edge versions
> > of tools in the BPF ecosystem including LLVM which are more involved
> > to satisfy than more typical requirements like installing a package from
> > your distribution.  This causes issues for users looking at kselftest in
> > as a whole who find that a default build of kselftest fails and that
> > resolving this is time consuming and adds administrative overhead.  The
> > fast pace of BPF development and the need for a full BPF stack to do
> > substantial development or validation work on the code mean that people
> > working directly on it don't see a reasonable way to keep supporting
> > older environments without causing problems with the usability of the
> > BPF tests in BPF development so these requirements are unlikely to be
> > relaxed in the immediate future.
> > 
> > There is already support for skipping targets so in order to reduce the
> > barrier to entry for people interested in kselftest as a whole let's use
> > that to skip the BPF tests by default when people work with the top
> > level kselftest build system.  Users can still build the BPF selftests
> > as part of the wider kselftest build by specifying SKIP_TARGETS,
> > including setting an empty SKIP_TARGETS to build everything.  They can
> > also continue to build the BPF selftests individually in cases where
> > they are specifically focused on BPF.
> > 
> > This isn't ideal since it means people will need to take special steps
> > to build the BPF tests but the dependencies mean that realistically this
> > is already the case to some extent and it makes it easier for people to
> > pick up and work with the other selftests which is hopefully a net win.
> > 
> > Signed-off-by: Mark Brown <broonie@kernel.org>
> > Reference: https://lore.kernel.org/bpf/20201210185233.28091-1-broonie@kernel.org/
> > Signed-off-by: Kamal Mostafa <kamal@canonical.com>
> 
> In our autotests we build the bpf selftests prior to running the net
> selftests, as some tests there require a module from the bpf selftests.
> With this patch it looks like we will need to change that to override
> SKIP_TARGETS so that bpf will not be filtered out. Is that correct?
> 
> Thanks,
> Seth
> 

I don't think that's correct ...

It depends on *how* we build the bpf selftests.  This patch ("selftests:
Skip BPF seftests by default") affects the regular mainline build
process and only matters if you try to 'make' all of
tools/testing/selftests/.

This patch does not affect a direct 'make' build of just
tools/testing/selftests/bpf/.

This patch does not affect the Ubuntu 'fdr compileselftests' target
(which uses its own list; the other patch in this series removes that).

Careful readers may notice that Ubuntu doesn't *really* need to pick up
this patch (since our regular build process does not try to 'make' all
of tools/testing/selftests) but I think it makes sense to do so.

So, I think its unlikely that our autotests build of the bpf selftests
would be affected SKIP_TARGETS.  If it is, it should probably be fixed
to build selftests/bpf directly instead.

 -Kamal


> > ---
> >  tools/testing/selftests/Makefile | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> > index 1195bd85af38..195177ba44ab 100644
> > --- a/tools/testing/selftests/Makefile
> > +++ b/tools/testing/selftests/Makefile
> > @@ -71,8 +71,10 @@ TARGETS += zram
> >  TARGETS_HOTPLUG = cpu-hotplug
> >  TARGETS_HOTPLUG += memory-hotplug
> >  
> > -# User can optionally provide a TARGETS skiplist.
> > -SKIP_TARGETS ?=
> > +# User can optionally provide a TARGETS skiplist.  By default we skip
> > +# BPF since it has cutting edge build time dependencies which require
> > +# more effort to install.
> > +SKIP_TARGETS ?= bpf
> >  ifneq ($(SKIP_TARGETS),)
> >  	TMP := $(filter-out $(SKIP_TARGETS), $(TARGETS))
> >  	override TARGETS := $(TMP)
> > -- 
> > 2.17.1
> > 
> > 
> > -- 
> > kernel-team mailing list
> > kernel-team@lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team
Seth Forshee Dec. 16, 2020, 2:05 a.m. UTC | #3 
On Tue, Dec 15, 2020 at 09:51:11AM -0800, Kamal Mostafa wrote:
> On Tue, Dec 15, 2020 at 07:52:10AM -0600, Seth Forshee wrote:
> > On Mon, Dec 14, 2020 at 02:00:39PM -0800, Kamal Mostafa wrote:
> > > From: Mark Brown <broonie@kernel.org>
> > > 
> > > BugLink: https://bugs.launchpad.net/bugs/1908144
> > > 
> > > The BPF selftests have build time dependencies on cutting edge versions
> > > of tools in the BPF ecosystem including LLVM which are more involved
> > > to satisfy than more typical requirements like installing a package from
> > > your distribution.  This causes issues for users looking at kselftest in
> > > as a whole who find that a default build of kselftest fails and that
> > > resolving this is time consuming and adds administrative overhead.  The
> > > fast pace of BPF development and the need for a full BPF stack to do
> > > substantial development or validation work on the code mean that people
> > > working directly on it don't see a reasonable way to keep supporting
> > > older environments without causing problems with the usability of the
> > > BPF tests in BPF development so these requirements are unlikely to be
> > > relaxed in the immediate future.
> > > 
> > > There is already support for skipping targets so in order to reduce the
> > > barrier to entry for people interested in kselftest as a whole let's use
> > > that to skip the BPF tests by default when people work with the top
> > > level kselftest build system.  Users can still build the BPF selftests
> > > as part of the wider kselftest build by specifying SKIP_TARGETS,
> > > including setting an empty SKIP_TARGETS to build everything.  They can
> > > also continue to build the BPF selftests individually in cases where
> > > they are specifically focused on BPF.
> > > 
> > > This isn't ideal since it means people will need to take special steps
> > > to build the BPF tests but the dependencies mean that realistically this
> > > is already the case to some extent and it makes it easier for people to
> > > pick up and work with the other selftests which is hopefully a net win.
> > > 
> > > Signed-off-by: Mark Brown <broonie@kernel.org>
> > > Reference: https://lore.kernel.org/bpf/20201210185233.28091-1-broonie@kernel.org/
> > > Signed-off-by: Kamal Mostafa <kamal@canonical.com>
> > 
> > In our autotests we build the bpf selftests prior to running the net
> > selftests, as some tests there require a module from the bpf selftests.
> > With this patch it looks like we will need to change that to override
> > SKIP_TARGETS so that bpf will not be filtered out. Is that correct?
> > 
> > Thanks,
> > Seth
> > 
> 
> I don't think that's correct ...
> 
> It depends on *how* we build the bpf selftests.  This patch ("selftests:
> Skip BPF seftests by default") affects the regular mainline build
> process and only matters if you try to 'make' all of
> tools/testing/selftests/.
> 
> This patch does not affect a direct 'make' build of just
> tools/testing/selftests/bpf/.
> 
> This patch does not affect the Ubuntu 'fdr compileselftests' target
> (which uses its own list; the other patch in this series removes that).
> 
> Careful readers may notice that Ubuntu doesn't *really* need to pick up
> this patch (since our regular build process does not try to 'make' all
> of tools/testing/selftests) but I think it makes sense to do so.
> 
> So, I think its unlikely that our autotests build of the bpf selftests
> would be affected SKIP_TARGETS.  If it is, it should probably be fixed
> to build selftests/bpf directly instead.

It builds it like this:

 make -C linux/tools/testing/selftests TARGETS=bpf

which is essentially the same as what the compileselftests target does.

As I'm reading that code, if SKIP_TARGETS isn't already set it will
explicitly override the value passed for TARGETS to have the bpf
selftest removed. Based on a little experimenting with the makefile,
that seems to be exactly what happens. Changing it to this:

 make -C tools/testing/selftests TARGETS=bpf SKIP_TARGETS=

leaves bpf in TARGETS.

Thanks,
Seth

> 
>  -Kamal
> 
> 
> > > ---
> > >  tools/testing/selftests/Makefile | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> > > index 1195bd85af38..195177ba44ab 100644
> > > --- a/tools/testing/selftests/Makefile
> > > +++ b/tools/testing/selftests/Makefile
> > > @@ -71,8 +71,10 @@ TARGETS += zram
> > >  TARGETS_HOTPLUG = cpu-hotplug
> > >  TARGETS_HOTPLUG += memory-hotplug
> > >  
> > > -# User can optionally provide a TARGETS skiplist.
> > > -SKIP_TARGETS ?=
> > > +# User can optionally provide a TARGETS skiplist.  By default we skip
> > > +# BPF since it has cutting edge build time dependencies which require
> > > +# more effort to install.
> > > +SKIP_TARGETS ?= bpf
> > >  ifneq ($(SKIP_TARGETS),)
> > >  	TMP := $(filter-out $(SKIP_TARGETS), $(TARGETS))
> > >  	override TARGETS := $(TMP)
> > > -- 
> > > 2.17.1
> > > 
> > > 
> > > -- 
> > > kernel-team mailing list
> > > kernel-team@lists.ubuntu.com
> > > https://lists.ubuntu.com/mailman/listinfo/kernel-team
Kamal Mostafa Dec. 16, 2020, 3:52 p.m. UTC | #4 
On Tue, Dec 15, 2020 at 6:05 PM Seth Forshee <seth.forshee@canonical.com>
wrote:

> On Tue, Dec 15, 2020 at 09:51:11AM -0800, Kamal Mostafa wrote:
> > On Tue, Dec 15, 2020 at 07:52:10AM -0600, Seth Forshee wrote:
> > > On Mon, Dec 14, 2020 at 02:00:39PM -0800, Kamal Mostafa wrote:
> > > > From: Mark Brown <broonie@kernel.org>
> > > >
> > > > BugLink: https://bugs.launchpad.net/bugs/1908144
> > > >
> > > > The BPF selftests have build time dependencies on cutting edge
> versions
>

<snip>


> It builds it like this:
>
>  make -C linux/tools/testing/selftests TARGETS=bpf
>
> which is essentially the same as what the compileselftests target does.
>
> As I'm reading that code, if SKIP_TARGETS isn't already set it will
> explicitly override the value passed for TARGETS to have the bpf
> selftest removed. Based on a little experimenting with the makefile,
> that seems to be exactly what happens. Changing it to this:
>
>  make -C tools/testing/selftests TARGETS=bpf SKIP_TARGETS=
>
> leaves bpf in TARGETS.
>

Why fiddle with TARGETS and SKIP_TARGETS at all?  Note that simply this
works -- with or without the disable patch(es) in place:

    make -C tools/testing/selftests/bpf

 -Kamal
Seth Forshee Dec. 16, 2020, 7:13 p.m. UTC | #5 
On Wed, Dec 16, 2020 at 07:52:37AM -0800, Kamal Mostafa wrote:
> On Tue, Dec 15, 2020 at 6:05 PM Seth Forshee <seth.forshee@canonical.com>
> wrote:
> 
> > On Tue, Dec 15, 2020 at 09:51:11AM -0800, Kamal Mostafa wrote:
> > > On Tue, Dec 15, 2020 at 07:52:10AM -0600, Seth Forshee wrote:
> > > > On Mon, Dec 14, 2020 at 02:00:39PM -0800, Kamal Mostafa wrote:
> > > > > From: Mark Brown <broonie@kernel.org>
> > > > >
> > > > > BugLink: https://bugs.launchpad.net/bugs/1908144
> > > > >
> > > > > The BPF selftests have build time dependencies on cutting edge
> > versions
> >
> 
> <snip>
> 
> 
> > It builds it like this:
> >
> >  make -C linux/tools/testing/selftests TARGETS=bpf
> >
> > which is essentially the same as what the compileselftests target does.
> >
> > As I'm reading that code, if SKIP_TARGETS isn't already set it will
> > explicitly override the value passed for TARGETS to have the bpf
> > selftest removed. Based on a little experimenting with the makefile,
> > that seems to be exactly what happens. Changing it to this:
> >
> >  make -C tools/testing/selftests TARGETS=bpf SKIP_TARGETS=
> >
> > leaves bpf in TARGETS.
> >
> 
> Why fiddle with TARGETS and SKIP_TARGETS at all?  Note that simply this
> works -- with or without the disable patch(es) in place:
> 
>     make -C tools/testing/selftests/bpf

Using TARGETS is the documented way of running a subset of the tests (as
per Documentation/dev-tools/kselftest.rst), and is the way all of our
scripting that I'm aware of does it. The above probably works, I haven't
tried it, but could be confusing to someone trying to work with our
kernel source who tries to run the bpf selftests by doing what the
documentation tells them to do. Plus, we have to update our scripting
which already uses TARGETS.

In my opinion, if you want to remove the bpf self tests from the default
set of targets, just remove the "TARGETS += bpf" line near the top of
the makefile. But I'm not sure why we need to do that. Do we have
scripting that runs kselftests without setting TARGETS? If we do, why
not update that to use SKIP_TARGETS instead?

Seth
diff mbox series

Patch

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 1195bd85af38..195177ba44ab 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -71,8 +71,10 @@  TARGETS += zram
 TARGETS_HOTPLUG = cpu-hotplug
 TARGETS_HOTPLUG += memory-hotplug
 
-# User can optionally provide a TARGETS skiplist.
-SKIP_TARGETS ?=
+# User can optionally provide a TARGETS skiplist.  By default we skip
+# BPF since it has cutting edge build time dependencies which require
+# more effort to install.
+SKIP_TARGETS ?= bpf
 ifneq ($(SKIP_TARGETS),)
 	TMP := $(filter-out $(SKIP_TARGETS), $(TARGETS))
 	override TARGETS := $(TMP)