Message-ID: <20180306171620.GD3382@kernel.org>
Date:   Tue, 6 Mar 2018 14:16:20 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     David Miller <davem@...emloft.net>, jbenc@...hat.com,
        netdev@...r.kernel.org, ast@...nel.org,
        jakub.kicinski@...ronome.com, acme@...hat.com,
        Ingo Molnar <mingo@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>
Subject: Re: [PATCH bpf] tools: bpftool: fix compilation with older headers

[ +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@...hat.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@...et 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@...et 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.

Powered by blists - more mailing lists