[bpf-next,v2,02/10] tools/bpf/runqslower: Fix override option for VMLINUX_BTF

Commit Message

Toke Høiland-Jørgensen Jan. 15, 2020, 2:12 p.m. UTC

From: Toke Høiland-Jørgensen <[e-mail]>

The runqslower tool refuses to build without a file to read vmlinux BTF
from. The build fails with an error message to override the location by
setting the VMLINUX_BTF variable if autodetection fails. However, the
Makefile doesn't actually work with that override - the error message is
still emitted.

Fix this by only doing auto-detection if no override is set. And while
we're at it, also look for a vmlinux file in the current kernel build dir
if none if found on the running kernel.

Fixes: 9c01546d26d2 ("tools/bpf: Add runqslower tool to tools/bpf")
Signed-off-by: Toke Høiland-Jørgensen <[e-mail]>
---
 tools/bpf/runqslower/Makefile |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Andrii Nakryiko Jan. 15, 2020, 4:59 p.m. UTC | #1

On Wed, Jan 15, 2020 at 6:13 AM Toke Høiland-Jørgensen <[e-mail]> wrote:
>
> From: Toke Høiland-Jørgensen <[e-mail]>
>
> The runqslower tool refuses to build without a file to read vmlinux BTF
> from. The build fails with an error message to override the location by
> setting the VMLINUX_BTF variable if autodetection fails. However, the
> Makefile doesn't actually work with that override - the error message is
> still emitted.

Do you have example command with VMLINUX_BTF override that didn't work
(and what error message was emitted)?

>
> Fix this by only doing auto-detection if no override is set. And while
> we're at it, also look for a vmlinux file in the current kernel build dir
> if none if found on the running kernel.
>
> Fixes: 9c01546d26d2 ("tools/bpf: Add runqslower tool to tools/bpf")
> Signed-off-by: Toke Høiland-Jørgensen <[e-mail]>
> ---
>  tools/bpf/runqslower/Makefile |   16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile
> index cff2fbcd29a8..fb93ce2bf2fe 100644
> --- a/tools/bpf/runqslower/Makefile
> +++ b/tools/bpf/runqslower/Makefile
> @@ -10,12 +10,16 @@ CFLAGS := -g -Wall
>
>  # Try to detect best kernel BTF source
>  KERNEL_REL := $(shell uname -r)
> -ifneq ("$(wildcard /sys/kernel/btf/vmlinux)","")
> -VMLINUX_BTF := /sys/kernel/btf/vmlinux
> -else ifneq ("$(wildcard /boot/vmlinux-$(KERNEL_REL))","")
> -VMLINUX_BTF := /boot/vmlinux-$(KERNEL_REL)
> -else
> -$(error "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly")
> +ifeq ("$(VMLINUX_BTF)","")
> +  ifneq ("$(wildcard /sys/kernel/btf/vmlinux)","")
> +  VMLINUX_BTF := /sys/kernel/btf/vmlinux
> +  else ifneq ("$(wildcard /boot/vmlinux-$(KERNEL_REL))","")
> +  VMLINUX_BTF := /boot/vmlinux-$(KERNEL_REL)
> +  else ifneq ("$(wildcard $(abspath ../../../vmlinux))","")
> +  VMLINUX_BTF := $(abspath ../../../vmlinux)

I'm planning to mirror runqslower into libbpf Github repo and this
../../../vmlinux piece will be completely out of place in that
context. Also it only will help when building kernel in-tree. So I'd
rather not add this.

> +  else
> +  $(error "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly")
> +  endif
>  endif
>
>  abs_out := $(abspath $(OUTPUT))
>

Toke Høiland-Jørgensen Jan. 15, 2020, 10:06 p.m. UTC | #2

Andrii Nakryiko <[e-mail]> writes:

> On Wed, Jan 15, 2020 at 6:13 AM Toke Høiland-Jørgensen <[e-mail]> wrote:
>>
>> From: Toke Høiland-Jørgensen <[e-mail]>
>>
>> The runqslower tool refuses to build without a file to read vmlinux BTF
>> from. The build fails with an error message to override the location by
>> setting the VMLINUX_BTF variable if autodetection fails. However, the
>> Makefile doesn't actually work with that override - the error message is
>> still emitted.
>
> Do you have example command with VMLINUX_BTF override that didn't work
> (and what error message was emitted)?

Before this patch:

$ cd ~/build/linux/tools/bpf/runqslower
$ make
Makefile:18: *** "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly".  Stop.

$ make VMLINUX_BTF=~/build/linux/vmlinux
Makefile:18: *** "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly".  Stop.

>> Fix this by only doing auto-detection if no override is set. And while
>> we're at it, also look for a vmlinux file in the current kernel build dir
>> if none if found on the running kernel.
>>
>> Fixes: 9c01546d26d2 ("tools/bpf: Add runqslower tool to tools/bpf")
>> Signed-off-by: Toke Høiland-Jørgensen <[e-mail]>
>> ---
>>  tools/bpf/runqslower/Makefile |   16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile
>> index cff2fbcd29a8..fb93ce2bf2fe 100644
>> --- a/tools/bpf/runqslower/Makefile
>> +++ b/tools/bpf/runqslower/Makefile
>> @@ -10,12 +10,16 @@ CFLAGS := -g -Wall
>>
>>  # Try to detect best kernel BTF source
>>  KERNEL_REL := $(shell uname -r)
>> -ifneq ("$(wildcard /sys/kernel/btf/vmlinux)","")
>> -VMLINUX_BTF := /sys/kernel/btf/vmlinux
>> -else ifneq ("$(wildcard /boot/vmlinux-$(KERNEL_REL))","")
>> -VMLINUX_BTF := /boot/vmlinux-$(KERNEL_REL)
>> -else
>> -$(error "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly")
>> +ifeq ("$(VMLINUX_BTF)","")
>> +  ifneq ("$(wildcard /sys/kernel/btf/vmlinux)","")
>> +  VMLINUX_BTF := /sys/kernel/btf/vmlinux
>> +  else ifneq ("$(wildcard /boot/vmlinux-$(KERNEL_REL))","")
>> +  VMLINUX_BTF := /boot/vmlinux-$(KERNEL_REL)
>> +  else ifneq ("$(wildcard $(abspath ../../../vmlinux))","")
>> +  VMLINUX_BTF := $(abspath ../../../vmlinux)
>
> I'm planning to mirror runqslower into libbpf Github repo and this
> ../../../vmlinux piece will be completely out of place in that
> context. Also it only will help when building kernel in-tree. So I'd
> rather not add this.

Well building the kernel in-tree is something people sometimes want to do ;)

Specifically, the selftests depend on this, so we should at least fix
those; but I guess it could work to just pass in VMLINUX_BTF as part of
the make -C from the selftests dir? I'll try that...

-Toke

Andrii Nakryiko Jan. 15, 2020, 10:31 p.m. UTC | #3

On Wed, Jan 15, 2020 at 2:06 PM Toke Høiland-Jørgensen <[e-mail]> wrote:
>
> Andrii Nakryiko <[e-mail]> writes:
>
> > On Wed, Jan 15, 2020 at 6:13 AM Toke Høiland-Jørgensen <[e-mail]> wrote:
> >>
> >> From: Toke Høiland-Jørgensen <[e-mail]>
> >>
> >> The runqslower tool refuses to build without a file to read vmlinux BTF
> >> from. The build fails with an error message to override the location by
> >> setting the VMLINUX_BTF variable if autodetection fails. However, the
> >> Makefile doesn't actually work with that override - the error message is
> >> still emitted.
> >
> > Do you have example command with VMLINUX_BTF override that didn't work
> > (and what error message was emitted)?
>
> Before this patch:
>
> $ cd ~/build/linux/tools/bpf/runqslower
> $ make
> Makefile:18: *** "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly".  Stop.
>
> $ make VMLINUX_BTF=~/build/linux/vmlinux
> Makefile:18: *** "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly".  Stop.

Ok, so this is strange. Try make clean and run with V=1, it might help
to debug this. This could happen if ~/build/linux/vmlinux doesn't
exist, but I assume you double-checked that. It works for me just fine
(Makefile won't do VMLINUX_BTF := assignment, if it's defined through
make invocation, so your change should be a no-op in that regard):

$ make clean
$ make VMLINUX_BTF=~/linux-build/default/vmlinux V=1
...
.output/sbin/bpftool btf dump file ~/linux-build/default/vmlinux
format c > .output/vmlinux.h
...

Wonder what your output looks like?

>
> >> Fix this by only doing auto-detection if no override is set. And while
> >> we're at it, also look for a vmlinux file in the current kernel build dir
> >> if none if found on the running kernel.
> >>
> >> Fixes: 9c01546d26d2 ("tools/bpf: Add runqslower tool to tools/bpf")
> >> Signed-off-by: Toke Høiland-Jørgensen <[e-mail]>
> >> ---
> >>  tools/bpf/runqslower/Makefile |   16 ++++++++++------
> >>  1 file changed, 10 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile
> >> index cff2fbcd29a8..fb93ce2bf2fe 100644
> >> --- a/tools/bpf/runqslower/Makefile
> >> +++ b/tools/bpf/runqslower/Makefile
> >> @@ -10,12 +10,16 @@ CFLAGS := -g -Wall
> >>
> >>  # Try to detect best kernel BTF source
> >>  KERNEL_REL := $(shell uname -r)
> >> -ifneq ("$(wildcard /sys/kernel/btf/vmlinux)","")
> >> -VMLINUX_BTF := /sys/kernel/btf/vmlinux
> >> -else ifneq ("$(wildcard /boot/vmlinux-$(KERNEL_REL))","")
> >> -VMLINUX_BTF := /boot/vmlinux-$(KERNEL_REL)
> >> -else
> >> -$(error "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly")
> >> +ifeq ("$(VMLINUX_BTF)","")
> >> +  ifneq ("$(wildcard /sys/kernel/btf/vmlinux)","")
> >> +  VMLINUX_BTF := /sys/kernel/btf/vmlinux
> >> +  else ifneq ("$(wildcard /boot/vmlinux-$(KERNEL_REL))","")
> >> +  VMLINUX_BTF := /boot/vmlinux-$(KERNEL_REL)
> >> +  else ifneq ("$(wildcard $(abspath ../../../vmlinux))","")
> >> +  VMLINUX_BTF := $(abspath ../../../vmlinux)
> >
> > I'm planning to mirror runqslower into libbpf Github repo and this
> > ../../../vmlinux piece will be completely out of place in that
> > context. Also it only will help when building kernel in-tree. So I'd
> > rather not add this.
>
> Well building the kernel in-tree is something people sometimes want to do ;)
>
> Specifically, the selftests depend on this, so we should at least fix
> those; but I guess it could work to just pass in VMLINUX_BTF as part of
> the make -C from the selftests dir? I'll try that...

Yes, it can be handled through VMLINUX_BTF override for selftests. As
I said, this will be a self-contained example in libbpf's Github repo,
so this "in kernel tree" assumption doesn't stand there.


>
> -Toke
>

Toke Høiland-Jørgensen Jan. 16, 2020, 9:05 a.m. UTC | #4

Andrii Nakryiko <[e-mail]> writes:

> On Wed, Jan 15, 2020 at 2:06 PM Toke Høiland-Jørgensen <[e-mail]> wrote:
>>
>> Andrii Nakryiko <[e-mail]> writes:
>>
>> > On Wed, Jan 15, 2020 at 6:13 AM Toke Høiland-Jørgensen <[e-mail]> wrote:
>> >>
>> >> From: Toke Høiland-Jørgensen <[e-mail]>
>> >>
>> >> The runqslower tool refuses to build without a file to read vmlinux BTF
>> >> from. The build fails with an error message to override the location by
>> >> setting the VMLINUX_BTF variable if autodetection fails. However, the
>> >> Makefile doesn't actually work with that override - the error message is
>> >> still emitted.
>> >
>> > Do you have example command with VMLINUX_BTF override that didn't work
>> > (and what error message was emitted)?
>>
>> Before this patch:
>>
>> $ cd ~/build/linux/tools/bpf/runqslower
>> $ make
>> Makefile:18: *** "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly".  Stop.
>>
>> $ make VMLINUX_BTF=~/build/linux/vmlinux
>> Makefile:18: *** "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly".  Stop.
>
> Ok, so this is strange. Try make clean and run with V=1, it might help
> to debug this. This could happen if ~/build/linux/vmlinux doesn't
> exist, but I assume you double-checked that. It works for me just fine
> (Makefile won't do VMLINUX_BTF := assignment, if it's defined through
> make invocation, so your change should be a no-op in that regard):
>
> $ make clean
> $ make VMLINUX_BTF=~/linux-build/default/vmlinux V=1
> ...
> .output/sbin/bpftool btf dump file ~/linux-build/default/vmlinux
> format c > .output/vmlinux.h
> ...
>
> Wonder what your output looks like?

$ make clean
Makefile:18: *** "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly".  Stop.
$ make VMLINUX_BTF=~/build/linux/vmlinux V=1
Makefile:18: *** "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly".  Stop.

Take another look at the relevant part of the makefile:

  ifneq ("$(wildcard /sys/kernel/btf/vmlinux)","")
  VMLINUX_BTF := /sys/kernel/btf/vmlinux
  else ifneq ("$(wildcard /boot/vmlinux-$(KERNEL_REL))","")
  VMLINUX_BTF := /boot/vmlinux-$(KERNEL_REL)
  else
  $(error "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly")
  endif

That if/else doesn't actually consider the value of VMLINUX_BTF; so the
override only works if one of the files being considered by the
auto-detection actually exists... :)

-Toke

Andrii Nakryiko Jan. 16, 2020, 5:14 p.m. UTC | #5

On Thu, Jan 16, 2020 at 1:05 AM Toke Høiland-Jørgensen <[e-mail]> wrote:
>
> Andrii Nakryiko <[e-mail]> writes:
>
> > On Wed, Jan 15, 2020 at 2:06 PM Toke Høiland-Jørgensen <[e-mail]> wrote:
> >>
> >> Andrii Nakryiko <[e-mail]> writes:
> >>
> >> > On Wed, Jan 15, 2020 at 6:13 AM Toke Høiland-Jørgensen <[e-mail]> wrote:
> >> >>
> >> >> From: Toke Høiland-Jørgensen <[e-mail]>
> >> >>
> >> >> The runqslower tool refuses to build without a file to read vmlinux BTF
> >> >> from. The build fails with an error message to override the location by
> >> >> setting the VMLINUX_BTF variable if autodetection fails. However, the
> >> >> Makefile doesn't actually work with that override - the error message is
> >> >> still emitted.
> >> >
> >> > Do you have example command with VMLINUX_BTF override that didn't work
> >> > (and what error message was emitted)?
> >>
> >> Before this patch:
> >>
> >> $ cd ~/build/linux/tools/bpf/runqslower
> >> $ make
> >> Makefile:18: *** "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly".  Stop.
> >>
> >> $ make VMLINUX_BTF=~/build/linux/vmlinux
> >> Makefile:18: *** "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly".  Stop.
> >
> > Ok, so this is strange. Try make clean and run with V=1, it might help
> > to debug this. This could happen if ~/build/linux/vmlinux doesn't
> > exist, but I assume you double-checked that. It works for me just fine
> > (Makefile won't do VMLINUX_BTF := assignment, if it's defined through
> > make invocation, so your change should be a no-op in that regard):
> >
> > $ make clean
> > $ make VMLINUX_BTF=~/linux-build/default/vmlinux V=1
> > ...
> > .output/sbin/bpftool btf dump file ~/linux-build/default/vmlinux
> > format c > .output/vmlinux.h
> > ...
> >
> > Wonder what your output looks like?
>
> $ make clean
> Makefile:18: *** "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly".  Stop.
> $ make VMLINUX_BTF=~/build/linux/vmlinux V=1
> Makefile:18: *** "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly".  Stop.
>
> Take another look at the relevant part of the makefile:
>
>   ifneq ("$(wildcard /sys/kernel/btf/vmlinux)","")
>   VMLINUX_BTF := /sys/kernel/btf/vmlinux
>   else ifneq ("$(wildcard /boot/vmlinux-$(KERNEL_REL))","")
>   VMLINUX_BTF := /boot/vmlinux-$(KERNEL_REL)
>   else
>   $(error "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly")
>   endif
>
> That if/else doesn't actually consider the value of VMLINUX_BTF; so the
> override only works if one of the files being considered by the
> auto-detection actually exists... :)

Ah, right, unconditional $(error), completely missed that few times, thanks!

>
> -Toke
>

Patch

diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile
index cff2fbcd29a8..fb93ce2bf2fe 100644
--- a/tools/bpf/runqslower/Makefile
+++ b/tools/bpf/runqslower/Makefile
@@ -10,12 +10,16 @@  CFLAGS := -g -Wall
 
 # Try to detect best kernel BTF source
 KERNEL_REL := $(shell uname -r)
-ifneq ("$(wildcard /sys/kernel/btf/vmlinux)","")
-VMLINUX_BTF := /sys/kernel/btf/vmlinux
-else ifneq ("$(wildcard /boot/vmlinux-$(KERNEL_REL))","")
-VMLINUX_BTF := /boot/vmlinux-$(KERNEL_REL)
-else
-$(error "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly")
+ifeq ("$(VMLINUX_BTF)","")
+  ifneq ("$(wildcard /sys/kernel/btf/vmlinux)","")
+  VMLINUX_BTF := /sys/kernel/btf/vmlinux
+  else ifneq ("$(wildcard /boot/vmlinux-$(KERNEL_REL))","")
+  VMLINUX_BTF := /boot/vmlinux-$(KERNEL_REL)
+  else ifneq ("$(wildcard $(abspath ../../../vmlinux))","")
+  VMLINUX_BTF := $(abspath ../../../vmlinux)
+  else
+  $(error "Can't detect kernel BTF, use VMLINUX_BTF to specify it explicitly")
+  endif
 endif
 
 abs_out := $(abspath $(OUTPUT))