| Message ID | 2018378f6cdd5f152ea597071c69b8e0874c769a.1520343947.git.jbenc@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] tools: bpftool: fix compilation with older headers | expand |
On 03/06/2018 02:50 PM, Jiri Benc wrote: > Compilation of bpftool on a distro that lacks eBPF support in the installed > kernel headers fails with: > > common.c: In function ‘is_bpffs’: > common.c:96:40: error: ‘BPF_FS_MAGIC’ undeclared (first use in this function) > return (unsigned long)st_fs.f_type == BPF_FS_MAGIC; > ^ > Fix this the same way it is already in tools/lib/bpf/libbpf.c and > tools/lib/api/fs/fs.c. > > Signed-off-by: Jiri Benc <jbenc@redhat.com> Thanks for the fix, Jiri! The standard approach to resolve such header dependencies under tools/ would be to add a copy of magic.h uapi header into tools/include/uapi/linux/magic.h. Both bpftool and libbpf have tools/include/uapi/ in their include path from their Makefile, so they would pull this in automatically and it would also allow to get rid of the extra ifdef in libbpf then. Could you look into that? Thanks, Daniel > --- > tools/bpf/bpftool/common.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c > index 0b482c0070e0..465995281dcd 100644 > --- a/tools/bpf/bpftool/common.c > +++ b/tools/bpf/bpftool/common.c > @@ -55,6 +55,10 @@ > > #include "main.h" > > +#ifndef BPF_FS_MAGIC > +#define BPF_FS_MAGIC 0xcafe4a11 > +#endif > + > void p_err(const char *fmt, ...) > { > va_list ap; >
On Tue, 6 Mar 2018 15:39:07 +0100, Daniel Borkmann wrote: > Thanks for the fix, Jiri! The standard approach to resolve such header dependencies under > tools/ would be to add a copy of magic.h uapi header into tools/include/uapi/linux/magic.h. > > Both bpftool and libbpf have tools/include/uapi/ in their include path from their > Makefile, so they would pull this in automatically and it would also allow to get rid > of the extra ifdef in libbpf then. Could you look into that? That's what I tried at first. But honestly, this is a shortcut to hell. Eventually, we'd end up with most of uapi headers duplicated under tools/include/uapi and hopelessly out of sync. The right approach would be to export uapi headers from the currently built kernel somewhere (a temporary directory, perhaps) and use that to build the tools. We should not have duplicated and out of sync headers in the kernel tree. Just look at the git log for tools/include/uapi to see what I mean by "out of sync". But that's certainly not a fix suitable for bpf.git, while I think the build problem should be fixed in bpf.git. We can do something more clever in bpf-next. I can of course add the magic.h header if you have strong opinion about that. I just don't think it's the right approach and seeing the precedent in tools/lib/bpf/libbpf.c and tools/lib/api/fs/fs.c, I think this minimal patch is better at this point until the duplicate headers mess is sorted out. Please let me know if you really want me to duplicate the magic.h file. Thanks, Jiri
From: Jiri Benc <jbenc@redhat.com> Date: Tue, 6 Mar 2018 16:03:25 +0100 > On Tue, 6 Mar 2018 15:39:07 +0100, Daniel Borkmann wrote: >> Thanks for the fix, Jiri! The standard approach to resolve such header dependencies under >> tools/ would be to add a copy of magic.h uapi header into tools/include/uapi/linux/magic.h. >> >> Both bpftool and libbpf have tools/include/uapi/ in their include path from their >> Makefile, so they would pull this in automatically and it would also allow to get rid >> of the extra ifdef in libbpf then. Could you look into that? > > That's what I tried at first. But honestly, this is a shortcut to hell. > Eventually, we'd end up with most of uapi headers duplicated under > tools/include/uapi and hopelessly out of sync. > > The right approach would be to export uapi headers from the currently > built kernel somewhere (a temporary directory, perhaps) and use that to > build the tools. We should not have duplicated and out of sync headers > in the kernel tree. Just look at the git log for tools/include/uapi to > see what I mean by "out of sync". I understand your frustration. I'm really puzzled why doing "make headers_install" and then building these tools does not pick those in-kernel headers up. That's what really should happen. The kernel tree internally should be self-consistent. It's one thing for an external tool like iproute2 to duplicate stuff like this, but user programs inside the kernel have no excuse for requiring things like that just to build.
[ +acme ] On 03/06/2018 05:00 PM, David Miller wrote: > From: Jiri Benc <jbenc@redhat.com> > Date: Tue, 6 Mar 2018 16:03:25 +0100 > >> On Tue, 6 Mar 2018 15:39:07 +0100, Daniel Borkmann wrote: >>> Thanks for the fix, Jiri! The standard approach to resolve such header dependencies under >>> tools/ would be to add a copy of magic.h uapi header into tools/include/uapi/linux/magic.h. >>> >>> Both bpftool and libbpf have tools/include/uapi/ in their include path from their >>> Makefile, so they would pull this in automatically and it would also allow to get rid >>> of the extra ifdef in libbpf then. Could you look into that? >> >> That's what I tried at first. But honestly, this is a shortcut to hell. >> Eventually, we'd end up with most of uapi headers duplicated under >> tools/include/uapi and hopelessly out of sync. >> >> The right approach would be to export uapi headers from the currently >> built kernel somewhere (a temporary directory, perhaps) and use that to >> build the tools. We should not have duplicated and out of sync headers >> in the kernel tree. Just look at the git log for tools/include/uapi to >> see what I mean by "out of sync". > > I understand your frustration. > > I'm really puzzled why doing "make headers_install" and then building > these tools does not pick those in-kernel headers up. That's what > really should happen. Arnaldo, given this came out of tools/perf originally and duplicating/syncing headers is common practice since about 2014 in kernel git tree, do you have some context on why the above was/is not considered? My current understanding is that the general preference would be on copying the headers into tools/include/ infrastructure once there are dependencies identified that would be missing on older/local system headers rather than ifdef'ery of various bit and pieces in the code that need to make use of them. Would be good to get some clarification on that in any case. But that said, I'd also be fine taking the three-liner as is into bpf as a fix. > The kernel tree internally should be self-consistent. > > It's one thing for an external tool like iproute2 to duplicate stuff > like this, but user programs inside the kernel have no excuse for > requiring things like that just to build.
[ +ingo, jolsa, namhyung ] Em Tue, Mar 06, 2018 at 05:28:33PM +0100, Daniel Borkmann escreveu: > [ +acme ] > > On 03/06/2018 05:00 PM, David Miller wrote: > > From: Jiri Benc <jbenc@redhat.com> > > Date: Tue, 6 Mar 2018 16:03:25 +0100 > > > >> On Tue, 6 Mar 2018 15:39:07 +0100, Daniel Borkmann wrote: > >>> Thanks for the fix, Jiri! The standard approach to resolve such header dependencies under > >>> tools/ would be to add a copy of magic.h uapi header into tools/include/uapi/linux/magic.h. > >>> > >>> Both bpftool and libbpf have tools/include/uapi/ in their include path from their > >>> Makefile, so they would pull this in automatically and it would also allow to get rid > >>> of the extra ifdef in libbpf then. Could you look into that? > >> > >> That's what I tried at first. But honestly, this is a shortcut to hell. > >> Eventually, we'd end up with most of uapi headers duplicated under > >> tools/include/uapi and hopelessly out of sync. > >> > >> The right approach would be to export uapi headers from the currently > >> built kernel somewhere (a temporary directory, perhaps) and use that to > >> build the tools. We should not have duplicated and out of sync headers > >> in the kernel tree. Just look at the git log for tools/include/uapi to > >> see what I mean by "out of sync". > > > > I understand your frustration. > > > > I'm really puzzled why doing "make headers_install" and then building > > these tools does not pick those in-kernel headers up. That's what > > really should happen. > > Arnaldo, given this came out of tools/perf originally and duplicating/syncing > headers is common practice since about 2014 in kernel git tree, do you have > some context on why the above was/is not considered? So, when tools/perf/ started we tried to use kernel code directly, things like rbtree, list.h, etc. It worked, we were sharing stuff with the kernel, all is well. Then someone changes something in one of these files and tools/ compilation broke. Fine for tools/ developers, we knew something like that could happen and would fix things in tools/, life goes on. Then Linus, IIRC, tried building tools/perf/ when something like that had happened and the build broke for him. He didn't liked that and we came up with this copy and check thing: we copy something into tools/include/ and add it to tools/perf/check-headers.sh, when something changes in the kernel, nothing breaks, we get notified, check if the change implies changes in tools/perf/, things like improving 'perf trace' to deal with new ioctl or syscall flags, new syscalls, etc. Sometimes the copy ends up automatically updating the tools, as there are scripts that generate ioctl id->string tables, for instance, automatically from the updated headers, things like what happened in this header update: http://git.kernel.org/acme/c/1350fb7d1b48 tools include powerpc: Grab a copy of arch/powerpc/include/uapi/asm/unistd.h With this in place no kernel developer needs to care about what happens in tools/, tools/ developers don't need to worry about getting in the way of kernel developers day-to-day activities. Then we also have: [acme@jouet perf]$ make help | grep perf perf-tar-src-pkg - Build perf-4.16.0-rc4.tar source tarball perf-targz-src-pkg - Build perf-4.16.0-rc4.tar.gz source tarball perf-tarbz2-src-pkg - Build perf-4.16.0-rc4.tar.bz2 source tarball perf-tarxz-src-pkg - Build perf-4.16.0-rc4.tar.xz source tarball [acme@jouet perf]$ Use that, get the resulting tarball, and all you need to build it anywhere should be self contained there, so the tools may use flags, defines, syscall definitions, etc, without ifdefs, and the resulting source code will build in many places, cross compiling, etc, like is done for every pull request I send to Ingo, see for instance the 53 containers where this is all built in a pull req like the one from yesterday: http://lkml.kernel.org/r/20180305142932.16921-1-acme@kernel.org But now there are more tools in tools/ and of course we can and should improve the whole process in a way that satisfies the various projects. So, with this said, I'll try and read the above thread. Ingo may add some other thoughts here, this is what came to my mind now. - Arnaldo > My current understanding is that the general preference would be on copying > the headers into tools/include/ infrastructure once there are dependencies > identified that would be missing on older/local system headers rather than > ifdef'ery of various bit and pieces in the code that need to make use of them. > Would be good to get some clarification on that in any case. > > But that said, I'd also be fine taking the three-liner as is into bpf as a fix. > > > The kernel tree internally should be self-consistent. > > > > It's one thing for an external tool like iproute2 to duplicate stuff > > like this, but user programs inside the kernel have no excuse for > > requiring things like that just to build.
On 03/06/2018 02:50 PM, Jiri Benc wrote: > Compilation of bpftool on a distro that lacks eBPF support in the installed > kernel headers fails with: > > common.c: In function ‘is_bpffs’: > common.c:96:40: error: ‘BPF_FS_MAGIC’ undeclared (first use in this function) > return (unsigned long)st_fs.f_type == BPF_FS_MAGIC; > ^ > Fix this the same way it is already in tools/lib/bpf/libbpf.c and > tools/lib/api/fs/fs.c. > > Signed-off-by: Jiri Benc <jbenc@redhat.com> Applied it to bpf tree, thanks Jiri! If preferred by Arnaldo/Ingo/others we can still pull in magic.h later on in bpf-next and get rid of the ifdefs, not a big thing either.
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c index 0b482c0070e0..465995281dcd 100644 --- a/tools/bpf/bpftool/common.c +++ b/tools/bpf/bpftool/common.c @@ -55,6 +55,10 @@ #include "main.h" +#ifndef BPF_FS_MAGIC +#define BPF_FS_MAGIC 0xcafe4a11 +#endif + void p_err(const char *fmt, ...) { va_list ap;
Compilation of bpftool on a distro that lacks eBPF support in the installed kernel headers fails with: common.c: In function ‘is_bpffs’: common.c:96:40: error: ‘BPF_FS_MAGIC’ undeclared (first use in this function) return (unsigned long)st_fs.f_type == BPF_FS_MAGIC; ^ Fix this the same way it is already in tools/lib/bpf/libbpf.c and tools/lib/api/fs/fs.c. Signed-off-by: Jiri Benc <jbenc@redhat.com> --- tools/bpf/bpftool/common.c | 4 ++++ 1 file changed, 4 insertions(+)