| Message ID | 20210125154938.40504-1-quentin@isovalent.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: fix build for BPF preload when $(O) points to a relative path | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, Jan 25, 2021 at 7:49 AM Quentin Monnet <quentin@isovalent.com> wrote: > > Building the kernel with CONFIG_BPF_PRELOAD, and by providing a relative > path for the output directory, may fail with the following error: > > $ make O=build bindeb-pkg > ... > /.../linux/tools/scripts/Makefile.include:5: *** O=build does not exist. Stop. > make[7]: *** [/.../linux/kernel/bpf/preload/Makefile:9: kernel/bpf/preload/libbpf.a] Error 2 > make[6]: *** [/.../linux/scripts/Makefile.build:500: kernel/bpf/preload] Error 2 > make[5]: *** [/.../linux/scripts/Makefile.build:500: kernel/bpf] Error 2 > make[4]: *** [/.../linux/Makefile:1799: kernel] Error 2 > make[4]: *** Waiting for unfinished jobs.... > > In the case above, for the "bindeb-pkg" target, the error is produced by > the "dummy" check in Makefile.include, called from libbpf's Makefile. > This check changes directory to $(PWD) before checking for the existence > of $(O). But at this step we have $(PWD) pointing to "/.../linux/build", > and $(O) pointing to "build". So the Makefile.include tries in fact to > assert the existence of a directory named "/.../linux/build/build", > which does not exist. > > By contrast, other tools called from the main Linux Makefile get the > variable set to $(abspath $(objtree)), where $(objtree) is ".". We can > update the Makefile for kernel/bpf/preload to set $(O) to the same > value, to permit compiling with a relative path for output. Note that > apart from the Makefile.include, the variable $(O) is not used in > libbpf's build system. > > Note that the error does not occur for all make targets and > architectures combinations. > > - On x86, "make O=build vmlinux" appears to work fine. > $(PWD) points to "/.../linux/tools", but $(O) points to the absolute > path "/.../linux/build" and the test succeeds. > - On UML, it has been reported to fail with a message similar to the > above (see [0]). > - On x86, "make O=build bindeb-pkg" fails, as described above. > > It is unsure where the different values for $(O) and $(PWD) come from > (likely some recursive make with different arguments at some point), and > because several targets are broken, it feels safer to fix the $(O) value > passed to libbpf rather than to hunt down all changes to the variable. > > David Gow previously posted a slightly different version of this patch > as a RFC [0], two months ago or so. > > [0] https://lore.kernel.org/bpf/20201119085022.3606135-1-davidgow@google.com/t/#u > > Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> > Cc: Brendan Higgins <brendanhiggins@google.com> > Cc: David Gow <davidgow@google.com> > Reported-by: David Gow <davidgow@google.com> > Signed-off-by: Quentin Monnet <quentin@isovalent.com> > --- I still think it would benefit everyone to figure out where this is breaking (given Linux Makefile explicitly tries to handle such relative path situation for O=, I believe), but this is trivial enough, so: Acked-by: Andrii Nakryiko <andrii@kernel.org> BTW, you haven't specified which tree you intended it for. > kernel/bpf/preload/Makefile | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/preload/Makefile b/kernel/bpf/preload/Makefile > index 23ee310b6eb4..11b9896424c0 100644 > --- a/kernel/bpf/preload/Makefile > +++ b/kernel/bpf/preload/Makefile > @@ -4,8 +4,11 @@ LIBBPF_SRCS = $(srctree)/tools/lib/bpf/ > LIBBPF_A = $(obj)/libbpf.a > LIBBPF_OUT = $(abspath $(obj)) > > +# Set $(O) so that the "dummy" test in tools/scripts/Makefile.include, called > +# by libbpf's Makefile, succeeds when building the kernel with $(O) pointing to > +# a relative path, as in "make O=build bindeb-pkg". > $(LIBBPF_A): > - $(Q)$(MAKE) -C $(LIBBPF_SRCS) OUTPUT=$(LIBBPF_OUT)/ $(LIBBPF_OUT)/libbpf.a > + $(Q)$(MAKE) -C $(LIBBPF_SRCS) O=$(abspath .) OUTPUT=$(LIBBPF_OUT)/ $(LIBBPF_OUT)/libbpf.a why not O=$(LIBBPF_OUT), btw? > > userccflags += -I $(srctree)/tools/include/ -I $(srctree)/tools/include/uapi \ > -I $(srctree)/tools/lib/ -Wno-unused-result > -- > 2.25.1 >
On Mon, Jan 25, 2021 at 11:49 PM Quentin Monnet <quentin@isovalent.com> wrote: > > Building the kernel with CONFIG_BPF_PRELOAD, and by providing a relative > path for the output directory, may fail with the following error: > > $ make O=build bindeb-pkg > ... > /.../linux/tools/scripts/Makefile.include:5: *** O=build does not exist. Stop. > make[7]: *** [/.../linux/kernel/bpf/preload/Makefile:9: kernel/bpf/preload/libbpf.a] Error 2 > make[6]: *** [/.../linux/scripts/Makefile.build:500: kernel/bpf/preload] Error 2 > make[5]: *** [/.../linux/scripts/Makefile.build:500: kernel/bpf] Error 2 > make[4]: *** [/.../linux/Makefile:1799: kernel] Error 2 > make[4]: *** Waiting for unfinished jobs.... > > In the case above, for the "bindeb-pkg" target, the error is produced by > the "dummy" check in Makefile.include, called from libbpf's Makefile. > This check changes directory to $(PWD) before checking for the existence > of $(O). But at this step we have $(PWD) pointing to "/.../linux/build", > and $(O) pointing to "build". So the Makefile.include tries in fact to > assert the existence of a directory named "/.../linux/build/build", > which does not exist. > > By contrast, other tools called from the main Linux Makefile get the > variable set to $(abspath $(objtree)), where $(objtree) is ".". We can > update the Makefile for kernel/bpf/preload to set $(O) to the same > value, to permit compiling with a relative path for output. Note that > apart from the Makefile.include, the variable $(O) is not used in > libbpf's build system. > > Note that the error does not occur for all make targets and > architectures combinations. > > - On x86, "make O=build vmlinux" appears to work fine. > $(PWD) points to "/.../linux/tools", but $(O) points to the absolute > path "/.../linux/build" and the test succeeds. > - On UML, it has been reported to fail with a message similar to the > above (see [0]). > - On x86, "make O=build bindeb-pkg" fails, as described above. > > It is unsure where the different values for $(O) and $(PWD) come from > (likely some recursive make with different arguments at some point), and > because several targets are broken, it feels safer to fix the $(O) value > passed to libbpf rather than to hunt down all changes to the variable. > > David Gow previously posted a slightly different version of this patch > as a RFC [0], two months ago or so. > > [0] https://lore.kernel.org/bpf/20201119085022.3606135-1-davidgow@google.com/t/#u > > Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> > Cc: Brendan Higgins <brendanhiggins@google.com> > Cc: David Gow <davidgow@google.com> > Reported-by: David Gow <davidgow@google.com> > Signed-off-by: Quentin Monnet <quentin@isovalent.com> > --- Thanks for following up on this. I've tested it against my usecase (allyesconfig with ARCH=um), and it fixes the issue nicely. Tested-by: David Gow <davidgow@google.com> While I tend to agree it'd be nicer to track down the place $(O) and $(PWD) are broken, too, given that there seem to be a number of different ways to trigger this (bindeb-pkg and ARCH=um), I'm not sure there's only one breakage to find. Cheers, -- David
2021-01-25 16:32 UTC-0800 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > On Mon, Jan 25, 2021 at 7:49 AM Quentin Monnet <quentin@isovalent.com> wrote: >> >> Building the kernel with CONFIG_BPF_PRELOAD, and by providing a relative >> path for the output directory, may fail with the following error: >> >> $ make O=build bindeb-pkg >> ... >> /.../linux/tools/scripts/Makefile.include:5: *** O=build does not exist. Stop. >> make[7]: *** [/.../linux/kernel/bpf/preload/Makefile:9: kernel/bpf/preload/libbpf.a] Error 2 >> make[6]: *** [/.../linux/scripts/Makefile.build:500: kernel/bpf/preload] Error 2 >> make[5]: *** [/.../linux/scripts/Makefile.build:500: kernel/bpf] Error 2 >> make[4]: *** [/.../linux/Makefile:1799: kernel] Error 2 >> make[4]: *** Waiting for unfinished jobs.... >> >> In the case above, for the "bindeb-pkg" target, the error is produced by >> the "dummy" check in Makefile.include, called from libbpf's Makefile. >> This check changes directory to $(PWD) before checking for the existence >> of $(O). But at this step we have $(PWD) pointing to "/.../linux/build", >> and $(O) pointing to "build". So the Makefile.include tries in fact to >> assert the existence of a directory named "/.../linux/build/build", >> which does not exist. >> >> By contrast, other tools called from the main Linux Makefile get the >> variable set to $(abspath $(objtree)), where $(objtree) is ".". We can >> update the Makefile for kernel/bpf/preload to set $(O) to the same >> value, to permit compiling with a relative path for output. Note that >> apart from the Makefile.include, the variable $(O) is not used in >> libbpf's build system. >> >> Note that the error does not occur for all make targets and >> architectures combinations. >> >> - On x86, "make O=build vmlinux" appears to work fine. >> $(PWD) points to "/.../linux/tools", but $(O) points to the absolute >> path "/.../linux/build" and the test succeeds. >> - On UML, it has been reported to fail with a message similar to the >> above (see [0]). >> - On x86, "make O=build bindeb-pkg" fails, as described above. >> >> It is unsure where the different values for $(O) and $(PWD) come from >> (likely some recursive make with different arguments at some point), and >> because several targets are broken, it feels safer to fix the $(O) value >> passed to libbpf rather than to hunt down all changes to the variable. >> >> David Gow previously posted a slightly different version of this patch >> as a RFC [0], two months ago or so. >> >> [0] https://lore.kernel.org/bpf/20201119085022.3606135-1-davidgow@google.com/t/#u >> >> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> >> Cc: Brendan Higgins <brendanhiggins@google.com> >> Cc: David Gow <davidgow@google.com> >> Reported-by: David Gow <davidgow@google.com> >> Signed-off-by: Quentin Monnet <quentin@isovalent.com> >> --- > > I still think it would benefit everyone to figure out where this is > breaking (given Linux Makefile explicitly tries to handle such > relative path situation for O=, I believe), but this is trivial > enough, so: > > Acked-by: Andrii Nakryiko <andrii@kernel.org> Agreed, I'll try to spend a bit more time on this when I can. But it would be nice to have the fix in the meantime. Thanks for the review and ack. > > BTW, you haven't specified which tree you intended it for. Oops! I _knew_ I was missing something, sorry. This build issue is here since eBPF preload was introduced, so I meant to send to the *bpf* tree. Because it does not concern the major build targets, I was not sure if a "Fixes:" tag would be appropriate. If we want one, it should be for d71fa5c9763c ("bpf: Add kernel module with user mode driver that populates bpffs.") > >> kernel/bpf/preload/Makefile | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/bpf/preload/Makefile b/kernel/bpf/preload/Makefile >> index 23ee310b6eb4..11b9896424c0 100644 >> --- a/kernel/bpf/preload/Makefile >> +++ b/kernel/bpf/preload/Makefile >> @@ -4,8 +4,11 @@ LIBBPF_SRCS = $(srctree)/tools/lib/bpf/ >> LIBBPF_A = $(obj)/libbpf.a >> LIBBPF_OUT = $(abspath $(obj)) >> >> +# Set $(O) so that the "dummy" test in tools/scripts/Makefile.include, called >> +# by libbpf's Makefile, succeeds when building the kernel with $(O) pointing to >> +# a relative path, as in "make O=build bindeb-pkg". >> $(LIBBPF_A): >> - $(Q)$(MAKE) -C $(LIBBPF_SRCS) OUTPUT=$(LIBBPF_OUT)/ $(LIBBPF_OUT)/libbpf.a >> + $(Q)$(MAKE) -C $(LIBBPF_SRCS) O=$(abspath .) OUTPUT=$(LIBBPF_OUT)/ $(LIBBPF_OUT)/libbpf.a > > why not O=$(LIBBPF_OUT), btw? $(LIBBPF_OUT) points to /.../linux/ZZ_BUILD/build/kernel/bpf/preload. This is an absolute path so the "dummy" check should work, too. I preferred to align the value on the root Makefile, which has "O=$(abspath $(objtree))" for target "tools/%", but no strong opinion here. David would simply empty the variable in his patch. I'm fine with any of the three versions. Would you prefer me to resend with $(LIBBPF_OUT)? Thanks, Quentin
2021-01-26 11:24 UTC+0000 ~ Quentin Monnet <quentin@isovalent.com> > 2021-01-25 16:32 UTC-0800 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> >> On Mon, Jan 25, 2021 at 7:49 AM Quentin Monnet <quentin@isovalent.com> wrote: >>> >>> Building the kernel with CONFIG_BPF_PRELOAD, and by providing a relative >>> path for the output directory, may fail with the following error: >>> >>> $ make O=build bindeb-pkg >>> ... >>> /.../linux/tools/scripts/Makefile.include:5: *** O=build does not exist. Stop. >>> make[7]: *** [/.../linux/kernel/bpf/preload/Makefile:9: kernel/bpf/preload/libbpf.a] Error 2 >>> make[6]: *** [/.../linux/scripts/Makefile.build:500: kernel/bpf/preload] Error 2 >>> make[5]: *** [/.../linux/scripts/Makefile.build:500: kernel/bpf] Error 2 >>> make[4]: *** [/.../linux/Makefile:1799: kernel] Error 2 >>> make[4]: *** Waiting for unfinished jobs.... >>> >>> In the case above, for the "bindeb-pkg" target, the error is produced by >>> the "dummy" check in Makefile.include, called from libbpf's Makefile. >>> This check changes directory to $(PWD) before checking for the existence >>> of $(O). But at this step we have $(PWD) pointing to "/.../linux/build", >>> and $(O) pointing to "build". So the Makefile.include tries in fact to >>> assert the existence of a directory named "/.../linux/build/build", >>> which does not exist. >>> >>> By contrast, other tools called from the main Linux Makefile get the >>> variable set to $(abspath $(objtree)), where $(objtree) is ".". We can >>> update the Makefile for kernel/bpf/preload to set $(O) to the same >>> value, to permit compiling with a relative path for output. Note that >>> apart from the Makefile.include, the variable $(O) is not used in >>> libbpf's build system. >>> >>> Note that the error does not occur for all make targets and >>> architectures combinations. >>> >>> - On x86, "make O=build vmlinux" appears to work fine. >>> $(PWD) points to "/.../linux/tools", but $(O) points to the absolute >>> path "/.../linux/build" and the test succeeds. >>> - On UML, it has been reported to fail with a message similar to the >>> above (see [0]). >>> - On x86, "make O=build bindeb-pkg" fails, as described above. >>> >>> It is unsure where the different values for $(O) and $(PWD) come from >>> (likely some recursive make with different arguments at some point), and >>> because several targets are broken, it feels safer to fix the $(O) value >>> passed to libbpf rather than to hunt down all changes to the variable. >>> >>> David Gow previously posted a slightly different version of this patch >>> as a RFC [0], two months ago or so. >>> >>> [0] https://lore.kernel.org/bpf/20201119085022.3606135-1-davidgow@google.com/t/#u >>> >>> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> >>> Cc: Brendan Higgins <brendanhiggins@google.com> >>> Cc: David Gow <davidgow@google.com> >>> Reported-by: David Gow <davidgow@google.com> >>> Signed-off-by: Quentin Monnet <quentin@isovalent.com> >>> --- >> >> I still think it would benefit everyone to figure out where this is >> breaking (given Linux Makefile explicitly tries to handle such >> relative path situation for O=, I believe), but this is trivial >> enough, so: >> >> Acked-by: Andrii Nakryiko <andrii@kernel.org> > > Agreed, I'll try to spend a bit more time on this when I can. But it > would be nice to have the fix in the meantime. Thanks for the review and > ack. +Cc Masahiro Yamada Looking further into this, my understanding is the following. tools/scripts/Makefile.include contains this check: dummy := $(if $(shell cd $(PWD); test -d $(O) || \ echo $(O)),$(error O=$(O) does not exist),) ABSOLUTE_O := $(shell cd $(PWD); cd $(O) ; pwd) Note the use of $(PWD). As I understand, it is the shell environment variable, as it was set when the initial "make" command was run. This seems to be passed down to recursive calls to make. So if I type $ cd /linux $ make O=build vmlinux Then I get $(PWD) set to "/linux" and $(O) set to "build". The Makefile executes a submake from the output directory: # Invoke a second make in the output directory, passing relevant # variables __sub-make: $(Q)$(MAKE) -C $(abs_objtree) \ -f $(abs_srctree)/Makefile $(MAKECMDGOALS) But the variables are preserved. So far, so good. When I try to build "bindeb-pkg" instead: $ cd /linux $ make O=build bindeb-pkg Then I initially set $(PWD) and $(O) to the same values. They are preserved after the call to the submake, after we have changed to the output directory. But if I understand correctly, the "bindeb-pkg" target writes a new Makefile as build/debian/rules in scripts/package/mkdebian, and calls it _indirectly_ through dpkg-buildpackage, which does _not_ preserve $(PWD) (instead, it is reset to /linux/build, the current directory when calling the script). I end up with $(O) set to "build", and $(PWD) set to "/linux/build". The "dummy" check called for libbpf fails to find "/linux/build/build". Can we avoid using $(PWD) in the first place? I'm not sure how. It was added in commit be40920fbf10 ("tools: Let O= makes handle a relative path with -C option") to accommodate building perf with "-C", so we could not replace $(PWD) with $$PWD (shell value at the time the directive is executed) for example, the values will be different. Can we unset $(O) so that, when we call dpkg-buildpackage, it reflects the output directory relatively to /linux/build/? Or pass a value for $(PWD), so it is preserved? Maybe, I don't know. It could be done in scripts/package/mkdebian for example, by passing "O=''" to the make call. It seems that the packages build well with this change. But then we might need to check and update the other packages too (RPM, snap, perf archives), and identify if something similar might be happening for UML. I'm not sure this is worth the trouble at this point, if all we want is to fix the eBPF preloads? But I'm open to discussion if this is really the path we want to go. Fixing $(O) to pass the dummy check is easier. However, when reading the commits I noticed that my patch is incorrect. It would break on something like "make O=~/build bindeb-pkg", because abspath does not resolve special shell characters like "~". See also commit 028568d84da3 ("kbuild: revert $(realpath ...) to $(shell cd ... && /bin/pwd)"): this is why tools/scripts/Makefile.include still has an "ABSOLUTE_O" variable. So instead of setting "O=$(abspath .)", I'll send a v2 with Andrii's suggestion to use $(LIBBPF_OUT). Quentin
diff --git a/kernel/bpf/preload/Makefile b/kernel/bpf/preload/Makefile index 23ee310b6eb4..11b9896424c0 100644 --- a/kernel/bpf/preload/Makefile +++ b/kernel/bpf/preload/Makefile @@ -4,8 +4,11 @@ LIBBPF_SRCS = $(srctree)/tools/lib/bpf/ LIBBPF_A = $(obj)/libbpf.a LIBBPF_OUT = $(abspath $(obj)) +# Set $(O) so that the "dummy" test in tools/scripts/Makefile.include, called +# by libbpf's Makefile, succeeds when building the kernel with $(O) pointing to +# a relative path, as in "make O=build bindeb-pkg". $(LIBBPF_A): - $(Q)$(MAKE) -C $(LIBBPF_SRCS) OUTPUT=$(LIBBPF_OUT)/ $(LIBBPF_OUT)/libbpf.a + $(Q)$(MAKE) -C $(LIBBPF_SRCS) O=$(abspath .) OUTPUT=$(LIBBPF_OUT)/ $(LIBBPF_OUT)/libbpf.a userccflags += -I $(srctree)/tools/include/ -I $(srctree)/tools/include/uapi \ -I $(srctree)/tools/lib/ -Wno-unused-result
Building the kernel with CONFIG_BPF_PRELOAD, and by providing a relative path for the output directory, may fail with the following error: $ make O=build bindeb-pkg ... /.../linux/tools/scripts/Makefile.include:5: *** O=build does not exist. Stop. make[7]: *** [/.../linux/kernel/bpf/preload/Makefile:9: kernel/bpf/preload/libbpf.a] Error 2 make[6]: *** [/.../linux/scripts/Makefile.build:500: kernel/bpf/preload] Error 2 make[5]: *** [/.../linux/scripts/Makefile.build:500: kernel/bpf] Error 2 make[4]: *** [/.../linux/Makefile:1799: kernel] Error 2 make[4]: *** Waiting for unfinished jobs.... In the case above, for the "bindeb-pkg" target, the error is produced by the "dummy" check in Makefile.include, called from libbpf's Makefile. This check changes directory to $(PWD) before checking for the existence of $(O). But at this step we have $(PWD) pointing to "/.../linux/build", and $(O) pointing to "build". So the Makefile.include tries in fact to assert the existence of a directory named "/.../linux/build/build", which does not exist. By contrast, other tools called from the main Linux Makefile get the variable set to $(abspath $(objtree)), where $(objtree) is ".". We can update the Makefile for kernel/bpf/preload to set $(O) to the same value, to permit compiling with a relative path for output. Note that apart from the Makefile.include, the variable $(O) is not used in libbpf's build system. Note that the error does not occur for all make targets and architectures combinations. - On x86, "make O=build vmlinux" appears to work fine. $(PWD) points to "/.../linux/tools", but $(O) points to the absolute path "/.../linux/build" and the test succeeds. - On UML, it has been reported to fail with a message similar to the above (see [0]). - On x86, "make O=build bindeb-pkg" fails, as described above. It is unsure where the different values for $(O) and $(PWD) come from (likely some recursive make with different arguments at some point), and because several targets are broken, it feels safer to fix the $(O) value passed to libbpf rather than to hunt down all changes to the variable. David Gow previously posted a slightly different version of this patch as a RFC [0], two months ago or so. [0] https://lore.kernel.org/bpf/20201119085022.3606135-1-davidgow@google.com/t/#u Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com> Cc: Brendan Higgins <brendanhiggins@google.com> Cc: David Gow <davidgow@google.com> Reported-by: David Gow <davidgow@google.com> Signed-off-by: Quentin Monnet <quentin@isovalent.com> --- kernel/bpf/preload/Makefile | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)