| Message ID | 20171220201943.24440-1-guro@fb.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [RFC,net-next] tools/bpftool: use version from the kernel source tree | expand |
On 12/20/17 12:19 PM, Roman Gushchin wrote: > Bpftool determines it's own version based on the kernel > version, which is picked from the linux/version.h header. > > It's strange to use the version of the installed kernel > headers, and makes much more sense to use the version > of the actual source tree, where bpftool sources are. > > This patch adds $(srctree)/usr/include to the list > of include files, which causes bpftool to use the version > from the source tree. > > Example: > before: > > $ bpftool version > bpftool v4.14.6 > > after: > $ bpftool version > bpftool v4.15.0 > > Signed-off-by: Roman Gushchin <guro@fb.com> > Cc: Jakub Kicinski <jakub.kicinski@netronome.com> > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Daniel Borkmann <daniel@iogearbox.net> > --- > tools/bpf/bpftool/Makefile | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile > index 9c089cfa5f3f..6864d416c49e 100644 > --- a/tools/bpf/bpftool/Makefile > +++ b/tools/bpf/bpftool/Makefile > @@ -37,7 +37,9 @@ CC = gcc > > CFLAGS += -O2 > CFLAGS += -W -Wall -Wextra -Wno-unused-parameter -Wshadow > -CFLAGS += -D__EXPORTED_HEADERS__ -I$(srctree)/tools/include/uapi -I$(srctree)/tools/include -I$(srctree)/tools/lib/bpf -I$(srctree)/kernel/bpf/ > +CFLAGS += -D__EXPORTED_HEADERS__ -I$(srctree)/tools/include/uapi > +CFLAGS += -I$(srctree)/tools/include -I$(srctree)/tools/lib/bpf > +CFLAGS += -I$(srctree)/kernel/bpf/ -I$(srctree)/usr/include -I$(srctree)/usr/include may not work if build directory is not the same as the source directory. You probably should use -I$(objtree)/usr/include? > LIBS = -lelf -lbfd -lopcodes $(LIBBPF) > > INSTALL ?= install >
On Wed, 20 Dec 2017 20:19:43 +0000, Roman Gushchin wrote: > Bpftool determines it's own version based on the kernel > version, which is picked from the linux/version.h header. > > It's strange to use the version of the installed kernel > headers, and makes much more sense to use the version > of the actual source tree, where bpftool sources are. > > This patch adds $(srctree)/usr/include to the list > of include files, which causes bpftool to use the version > from the source tree. > > Example: > before: > > $ bpftool version > bpftool v4.14.6 > > after: > $ bpftool version > bpftool v4.15.0 Thanks for the patch, this would indeed use some improvement. How about we just run make to get the version like liblockdep does? LIBLOCKDEP_VERSION=$(shell make --no-print-directory -sC ../../.. kernelversion) probably s@../../..@$(srctree)@ $(srctree)/usr/include is not going to be there for out-of-source builds. > Signed-off-by: Roman Gushchin <guro@fb.com> > Cc: Jakub Kicinski <jakub.kicinski@netronome.com> > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Daniel Borkmann <daniel@iogearbox.net> > --- > tools/bpf/bpftool/Makefile | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile > index 9c089cfa5f3f..6864d416c49e 100644 > --- a/tools/bpf/bpftool/Makefile > +++ b/tools/bpf/bpftool/Makefile > @@ -37,7 +37,9 @@ CC = gcc > > CFLAGS += -O2 > CFLAGS += -W -Wall -Wextra -Wno-unused-parameter -Wshadow > -CFLAGS += -D__EXPORTED_HEADERS__ -I$(srctree)/tools/include/uapi -I$(srctree)/tools/include -I$(srctree)/tools/lib/bpf -I$(srctree)/kernel/bpf/ > +CFLAGS += -D__EXPORTED_HEADERS__ -I$(srctree)/tools/include/uapi > +CFLAGS += -I$(srctree)/tools/include -I$(srctree)/tools/lib/bpf > +CFLAGS += -I$(srctree)/kernel/bpf/ -I$(srctree)/usr/include > LIBS = -lelf -lbfd -lopcodes $(LIBBPF) > > INSTALL ?= install
On Wed, Dec 20, 2017 at 12:29:21PM -0800, Jakub Kicinski wrote: > On Wed, 20 Dec 2017 20:19:43 +0000, Roman Gushchin wrote: > > Bpftool determines it's own version based on the kernel > > version, which is picked from the linux/version.h header. > > > > It's strange to use the version of the installed kernel > > headers, and makes much more sense to use the version > > of the actual source tree, where bpftool sources are. > > > > This patch adds $(srctree)/usr/include to the list > > of include files, which causes bpftool to use the version > > from the source tree. > > > > Example: > > before: > > > > $ bpftool version > > bpftool v4.14.6 > > > > after: > > $ bpftool version > > bpftool v4.15.0 > > Thanks for the patch, this would indeed use some improvement. > > How about we just run make to get the version like liblockdep does? > > LIBLOCKDEP_VERSION=$(shell make --no-print-directory -sC ../../.. kernelversion) > > probably s@../../..@$(srctree)@ > > $(srctree)/usr/include is not going to be there for out-of-source builds. Hm, why it's better? It's not only about the kernel version, IMO it's generally better to use includes from the source tree, rather then system-wide installed kernel headers. I've got about out-of-source builds, but do we support it in general? How can I build bpftool outside of the kernel tree? I've tried a bit, but failed. Thank you!
On Wed, Dec 20, 2017 at 12:26:30PM -0800, Yonghong Song wrote: > > > On 12/20/17 12:19 PM, Roman Gushchin wrote: > > Bpftool determines it's own version based on the kernel > > version, which is picked from the linux/version.h header. > > > > It's strange to use the version of the installed kernel > > headers, and makes much more sense to use the version > > of the actual source tree, where bpftool sources are. > > > > This patch adds $(srctree)/usr/include to the list > > of include files, which causes bpftool to use the version > > from the source tree. > > > > Example: > > before: > > > > $ bpftool version > > bpftool v4.14.6 > > > > after: > > $ bpftool version > > bpftool v4.15.0 > > > > Signed-off-by: Roman Gushchin <guro@fb.com> > > Cc: Jakub Kicinski <jakub.kicinski@netronome.com> > > Cc: Alexei Starovoitov <ast@kernel.org> > > Cc: Daniel Borkmann <daniel@iogearbox.net> > > --- > > tools/bpf/bpftool/Makefile | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile > > index 9c089cfa5f3f..6864d416c49e 100644 > > --- a/tools/bpf/bpftool/Makefile > > +++ b/tools/bpf/bpftool/Makefile > > @@ -37,7 +37,9 @@ CC = gcc > > CFLAGS += -O2 > > CFLAGS += -W -Wall -Wextra -Wno-unused-parameter -Wshadow > > -CFLAGS += -D__EXPORTED_HEADERS__ -I$(srctree)/tools/include/uapi -I$(srctree)/tools/include -I$(srctree)/tools/lib/bpf -I$(srctree)/kernel/bpf/ > > +CFLAGS += -D__EXPORTED_HEADERS__ -I$(srctree)/tools/include/uapi > > +CFLAGS += -I$(srctree)/tools/include -I$(srctree)/tools/lib/bpf > > +CFLAGS += -I$(srctree)/kernel/bpf/ -I$(srctree)/usr/include > > -I$(srctree)/usr/include may not work if build directory is not the same as > the source directory. You probably should use > -I$(objtree)/usr/include? $(objtree) is not defined there, so it doesn't work. Tbh, I struggle to say if it's supposed to work there or not. Thanks!
On Wed, 20 Dec 2017 20:53:41 +0000, Roman Gushchin wrote: > On Wed, Dec 20, 2017 at 12:29:21PM -0800, Jakub Kicinski wrote: > > On Wed, 20 Dec 2017 20:19:43 +0000, Roman Gushchin wrote: > > > Bpftool determines it's own version based on the kernel > > > version, which is picked from the linux/version.h header. > > > > > > It's strange to use the version of the installed kernel > > > headers, and makes much more sense to use the version > > > of the actual source tree, where bpftool sources are. > > > > > > This patch adds $(srctree)/usr/include to the list > > > of include files, which causes bpftool to use the version > > > from the source tree. > > > > > > Example: > > > before: > > > > > > $ bpftool version > > > bpftool v4.14.6 > > > > > > after: > > > $ bpftool version > > > bpftool v4.15.0 > > > > Thanks for the patch, this would indeed use some improvement. > > > > How about we just run make to get the version like liblockdep does? > > > > LIBLOCKDEP_VERSION=$(shell make --no-print-directory -sC ../../.. kernelversion) > > > > probably s@../../..@$(srctree)@ > > > > $(srctree)/usr/include is not going to be there for out-of-source builds. > > Hm, why it's better? It's not only about the kernel version, > IMO it's generally better to use includes from the source tree, > rather then system-wide installed kernel headers. Right I agree the kernel headers are preferred. I'm not entirely sure why we don't use them, if it was OK to assume usr/ is there we wouldn't need the tools/include/uapi/ contraption. Maybe Arnaldo could explain? > I've got about out-of-source builds, but do we support it in general? > How can I build bpftool outside of the kernel tree? > I've tried a bit, but failed. This is what I do: make -C tools/bpf/bpftool/ W=1 O=/tmp/builds/bpftool > > > diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile > > > index 9c089cfa5f3f..6864d416c49e 100644 > > > --- a/tools/bpf/bpftool/Makefile > > > +++ b/tools/bpf/bpftool/Makefile > > > @@ -37,7 +37,9 @@ CC = gcc > > > > > > CFLAGS += -O2 > > > CFLAGS += -W -Wall -Wextra -Wno-unused-parameter -Wshadow > > > -CFLAGS += -D__EXPORTED_HEADERS__ -I$(srctree)/tools/include/uapi -I$(srctree)/> > > tools/include -I$(srctree)/tools/lib/bpf -I$(srctree)/kernel/bpf/ > > > +CFLAGS += -D__EXPORTED_HEADERS__ -I$(srctree)/tools/include/uapi > > > +CFLAGS += -I$(srctree)/tools/include -I$(srctree)/tools/lib/bpf > > > +CFLAGS += -I$(srctree)/kernel/bpf/ -I$(srctree)/usr/include > > > LIBS = -lelf -lbfd -lopcodes $(LIBBPF) > > > > > > INSTALL ?= install
On Wed, Dec 20, 2017 at 01:52:18PM -0800, Jakub Kicinski wrote: > On Wed, 20 Dec 2017 20:53:41 +0000, Roman Gushchin wrote: > > On Wed, Dec 20, 2017 at 12:29:21PM -0800, Jakub Kicinski wrote: > > > On Wed, 20 Dec 2017 20:19:43 +0000, Roman Gushchin wrote: > > > > Bpftool determines it's own version based on the kernel > > > > version, which is picked from the linux/version.h header. > > > > > > > > It's strange to use the version of the installed kernel > > > > headers, and makes much more sense to use the version > > > > of the actual source tree, where bpftool sources are. > > > > > > > > This patch adds $(srctree)/usr/include to the list > > > > of include files, which causes bpftool to use the version > > > > from the source tree. > > > > > > > > Example: > > > > before: > > > > > > > > $ bpftool version > > > > bpftool v4.14.6 > > > > > > > > after: > > > > $ bpftool version > > > > bpftool v4.15.0 > > > > > > Thanks for the patch, this would indeed use some improvement. > > > > > > How about we just run make to get the version like liblockdep does? > > > > > > LIBLOCKDEP_VERSION=$(shell make --no-print-directory -sC ../../.. kernelversion) > > > > > > probably s@../../..@$(srctree)@ > > > > > > $(srctree)/usr/include is not going to be there for out-of-source builds. > > > > Hm, why it's better? It's not only about the kernel version, > > IMO it's generally better to use includes from the source tree, > > rather then system-wide installed kernel headers. > > Right I agree the kernel headers are preferred. I'm not entirely sure > why we don't use them, if it was OK to assume usr/ is there we wouldn't > need the tools/include/uapi/ contraption. Maybe Arnaldo could explain? > > > I've got about out-of-source builds, but do we support it in general? > > How can I build bpftool outside of the kernel tree? > > I've tried a bit, but failed. > > This is what I do: > > make -C tools/bpf/bpftool/ W=1 O=/tmp/builds/bpftool This works perfectly with my patch: $ make -C ~/linux/tools/bpf/ W=1 O=/home/guro/build/ --trace <...> echo ' CC '/home/guro/build/main.o;gcc -O2 -W -Wall -Wextra -Wno-unused-parameter -Wshadow -D__EXPORTED_HEADERS__ -I/home/guro/linux/tools/include/uapi -I/home/guro/linux/tools/include -I/home/guro/linux/tools/lib/bpf -I/home/guro/linux/kernel/bpf/ -I/home/guro/linux/usr/include -DNEW_DISSASSEMBLER_SIGNATURE -c -MMD -o /home/guro/build/main.o main.c <...> echo ' LINK '/home/guro/build/bpftool;gcc -O2 -W -Wall -Wextra -Wno-unused-parameter -Wshadow -D__EXPORTED_HEADERS__ -I/home/guro/linux/tools/include/uapi -I/home/guro/linux/tools/include -I/home/guro/linux/tools/lib/bpf -I/home/guro/linux/kernel/bpf/ -I/home/guro/linux/usr/include -DNEW_DISSASSEMBLER_SIGNATURE -o /home/guro/build/bpftool /home/guro/build/common.o /home/guro/build/cgroup.o /home/guro/build/main.o /home/guro/build/json_writer.o /home/guro/build/prog.o /home/guro/build/map.o /home/guro/build/jit_disasm.o /home/guro/build/disasm.o /home/guro/build/libbpf.a -lelf -lbfd -lopcodes /home/guro/build/libbpf.a LINK /home/guro/build/bpftool make[1]: Leaving directory '/home/guro/linux/tools/bpf/bpftool' make: Leaving directory '/home/guro/linux/tools/bpf' $ ./build/bpftool version ./build/bpftool v4.15.0 Thanks!
On Thu, 21 Dec 2017 12:07:42 +0000, Roman Gushchin wrote: > On Wed, Dec 20, 2017 at 01:52:18PM -0800, Jakub Kicinski wrote: > > On Wed, 20 Dec 2017 20:53:41 +0000, Roman Gushchin wrote: > > > On Wed, Dec 20, 2017 at 12:29:21PM -0800, Jakub Kicinski wrote: > > > Hm, why it's better? It's not only about the kernel version, > > > IMO it's generally better to use includes from the source tree, > > > rather then system-wide installed kernel headers. > > > > Right I agree the kernel headers are preferred. I'm not entirely sure > > why we don't use them, if it was OK to assume usr/ is there we wouldn't > > need the tools/include/uapi/ contraption. Maybe Arnaldo could explain? > > > > > I've got about out-of-source builds, but do we support it in general? > > > How can I build bpftool outside of the kernel tree? > > > I've tried a bit, but failed. > > > > This is what I do: > > > > make -C tools/bpf/bpftool/ W=1 O=/tmp/builds/bpftool > > This works perfectly with my patch: > > $ make -C ~/linux/tools/bpf/ W=1 O=/home/guro/build/ --trace > <...> > echo ' CC '/home/guro/build/main.o;gcc -O2 -W -Wall -Wextra -Wno-unused-parameter -Wshadow -D__EXPORTED_HEADERS__ -I/home/guro/linux/tools/include/uapi -I/home/guro/linux/tools/include -I/home/guro/linux/tools/lib/bpf -I/home/guro/linux/kernel/bpf/ -I/home/guro/linux/usr/include -DNEW_DISSASSEMBLER_SIGNATURE -c -MMD -o /home/guro/build/main.o main.c > <...> > echo ' LINK '/home/guro/build/bpftool;gcc -O2 -W -Wall -Wextra -Wno-unused-parameter -Wshadow -D__EXPORTED_HEADERS__ -I/home/guro/linux/tools/include/uapi -I/home/guro/linux/tools/include -I/home/guro/linux/tools/lib/bpf -I/home/guro/linux/kernel/bpf/ -I/home/guro/linux/usr/include -DNEW_DISSASSEMBLER_SIGNATURE -o /home/guro/build/bpftool /home/guro/build/common.o /home/guro/build/cgroup.o /home/guro/build/main.o /home/guro/build/json_writer.o /home/guro/build/prog.o /home/guro/build/map.o /home/guro/build/jit_disasm.o /home/guro/build/disasm.o /home/guro/build/libbpf.a -lelf -lbfd -lopcodes /home/guro/build/libbpf.a > LINK /home/guro/build/bpftool > make[1]: Leaving directory '/home/guro/linux/tools/bpf/bpftool' > make: Leaving directory '/home/guro/linux/tools/bpf' > > $ ./build/bpftool version > ./build/bpftool v4.15.0 Argh, sorry for the confusion you need to build the kernel out-of-source as well. In my case I build the kernel and bpftool out of source, and then the usr/ doesn't actually contain the auto-generated headers: $ ls ~/devel/linux/usr/ gen_init_cpio.c initramfs_data.S Kconfig Makefile Only build directory does: $ ls /tmp/builds/usr/ built-in.o gen_init_cpio include initramfs_data.cpio initramfs_data.o modules.builtin modules.order Let me reiterate, the user space headers we need should all be already included in -I$(srctree)/tools/include/uapi, and make kernelversion is nice because it also adds the -rc tags.
diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile index 9c089cfa5f3f..6864d416c49e 100644 --- a/tools/bpf/bpftool/Makefile +++ b/tools/bpf/bpftool/Makefile @@ -37,7 +37,9 @@ CC = gcc CFLAGS += -O2 CFLAGS += -W -Wall -Wextra -Wno-unused-parameter -Wshadow -CFLAGS += -D__EXPORTED_HEADERS__ -I$(srctree)/tools/include/uapi -I$(srctree)/tools/include -I$(srctree)/tools/lib/bpf -I$(srctree)/kernel/bpf/ +CFLAGS += -D__EXPORTED_HEADERS__ -I$(srctree)/tools/include/uapi +CFLAGS += -I$(srctree)/tools/include -I$(srctree)/tools/lib/bpf +CFLAGS += -I$(srctree)/kernel/bpf/ -I$(srctree)/usr/include LIBS = -lelf -lbfd -lopcodes $(LIBBPF) INSTALL ?= install
Bpftool determines it's own version based on the kernel version, which is picked from the linux/version.h header. It's strange to use the version of the installed kernel headers, and makes much more sense to use the version of the actual source tree, where bpftool sources are. This patch adds $(srctree)/usr/include to the list of include files, which causes bpftool to use the version from the source tree. Example: before: $ bpftool version bpftool v4.14.6 after: $ bpftool version bpftool v4.15.0 Signed-off-by: Roman Gushchin <guro@fb.com> Cc: Jakub Kicinski <jakub.kicinski@netronome.com> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Daniel Borkmann <daniel@iogearbox.net> --- tools/bpf/bpftool/Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)