[bpf-next,v4,00/12] install libbpf headers when using the library

Message

Quentin Monnet Oct. 7, 2021, 7:44 p.m. UTC

Libbpf is used at several locations in the repository. Most of the time,
the tools relying on it build the library in its own directory, and include
the headers from there. This works, but this is not the cleanest approach.
It generates objects outside of the directory of the tool which is being
built, and it also increases the risk that developers include a header file
internal to libbpf, which is not supposed to be exposed to user
applications.

This set adjusts all involved Makefiles to make sure that libbpf is built
locally (with respect to the tool's directory or provided build directory),
and by ensuring that "make install_headers" is run from libbpf's Makefile
to export user headers properly.

This comes at a cost: given that the libbpf was so far mostly compiled in
its own directory by the different components using it, compiling it once
would be enough for all those components. With the new approach, each
component compiles its own version. To mitigate this cost, efforts were
made to reuse the compiled library when possible:

- Make the bpftool version in samples/bpf reuse the library previously
  compiled for the selftests.
- Make the bpftool version in BPF selftests reuse the library previously
  compiled for the selftests.
- Similarly, make resolve_btfids in BPF selftests reuse the same compiled
  library.
- Similarly, make runqslower in BPF selftests reuse the same compiled
  library; and make it rely on the bpftool version also compiled from the
  selftests (instead of compiling its own version).
- runqslower, when compiled independently, needs its own version of
  bpftool: make them share the same compiled libbpf.

As a result:

- Compiling the samples/bpf should compile libbpf just once.
- Compiling the BPF selftests should compile libbpf just once.
- Compiling the kernel (with BTF support) should now lead to compiling
  libbpf twice: one for resolve_btfids, one for kernel/bpf/preload.
- Compiling runqslower individually should compile libbpf just once. Same
  thing for bpftool, resolve_btfids, and kernel/bpf/preload/iterators.

(Not accounting for the boostrap version of libbpf required by bpftool,
which was already placed under a dedicated .../boostrap/libbpf/ directory,
and for which the count remains unchanged.)

A few commits in the series also contain drive-by clean-up changes for
bpftool includes, samples/bpf/.gitignore, or test_bpftool_build.sh. Please
refer to individual commit logs for details.

v4:
  - Make the "libbpf_hdrs" dependency an order-only dependency in
    kernel/bpf/preload/Makefile, samples/bpf/Makefile, and
    tools/bpf/runqslower/Makefile. This is to avoid to unconditionally
    recompile the targets.
  - samples/bpf/.gitignore: prefix objects with a "/" to mark that we
    ignore them when at the root of the samples/bpf/ directory.
  - libbpf: add a commit to make "install_headers" depend on the header
    files, to avoid exporting again if the sources are older than the
    targets. This ensures that applications relying on those headers are
    not rebuilt unnecessarily.
  - bpftool: uncouple the copy of nlattr.h from libbpf target, to have it
    depend on the source header itself. By avoiding to reinstall this
    header every time, we avoid unnecessary builds of bpftool.
  - samples/bpf: Add a new commit to remove the FORCE dependency for
    libbpf, and replace it with a "$(wildcard ...)" on the .c/.h files in
    libbpf's directory. This is to avoid always recompiling libbpf/bpftool.
  - Adjust prefixes in commit subjects.

v3:
  - Remove order-only dependencies on $(LIBBPF_INCLUDE) (or equivalent)
    directories, given that they are created by libbpf's Makefile.
  - Add libbpf as a dependency for bpftool/resolve_btfids/runqslower when
    they are supposed to reuse a libbpf compiled previously. This is to
    avoid having several libbpf versions being compiled simultaneously in
    the same directory with parallel builds. Even if this didn't show up
    during tests, let's remain on the safe side.
  - kernel/bpf/preload/Makefile: Rename libbpf-hdrs (dash) dependency as
    libbpf_hdrs.
  - samples/bpf/.gitignore: Add bpftool/
  - samples/bpf/Makefile: Change "/bin/rm -rf" to "$(RM) -r".
  - samples/bpf/Makefile: Add missing slashes for $(LIBBPF_OUTPUT) and
    $(LIBBPF_DESTDIR) when buildling bpftool
  - samples/bpf/Makefile: Add a dependency to libbpf's headers for
    $(TRACE_HELPERS).
  - bpftool's Makefile: Use $(LIBBPF) instead of equivalent (but longer)
    $(LIBBPF_OUTPUT)libbpf.a
  - BPF iterators' Makefile: build bpftool in .output/bpftool (instead of
    .output/), add and clean up variables.
  - runqslower's Makefile: Add an explicit dependency on libbpf's headers
    to several objects. The dependency is not required (libbpf should have
    been compiled and so the headers exported through other dependencies
    for those targets), but they better mark the logical dependency and
    should help if exporting the headers changed in the future.
  - New commit to add an "install-bin" target to bpftool, to avoid
    installing bash completion when buildling BPF iterators and selftests.

v2: Declare an additional dependency on libbpf's headers for
    iterators/iterators.o in kernel/preload/Makefile to make sure that
    these headers are exported before we compile the object file (and not
    just before we link it).

Quentin Monnet (12):
  libbpf: skip re-installing headers file if source is older than target
  bpftool: remove unused includes to <bpf/bpf_gen_internal.h>
  bpftool: install libbpf headers instead of including the dir
  tools/resolve_btfids: install libbpf headers when building
  tools/runqslower: install libbpf headers when building
  bpf: preload: install libbpf headers when building
  bpf: iterators: install libbpf headers when building
  samples/bpf: update .gitignore
  samples/bpf: install libbpf headers when building
  samples/bpf: do not FORCE-recompile libbpf
  selftests/bpf: better clean up for runqslower in test_bpftool_build.sh
  bpftool: add install-bin target to install binary only

 kernel/bpf/preload/Makefile                   | 25 ++++++++---
 kernel/bpf/preload/iterators/Makefile         | 39 +++++++++++------
 samples/bpf/.gitignore                        |  4 ++
 samples/bpf/Makefile                          | 42 ++++++++++++++-----
 tools/bpf/bpftool/Makefile                    | 39 ++++++++++-------
 tools/bpf/bpftool/gen.c                       |  1 -
 tools/bpf/bpftool/prog.c                      |  1 -
 tools/bpf/resolve_btfids/Makefile             | 17 +++++---
 tools/bpf/resolve_btfids/main.c               |  4 +-
 tools/bpf/runqslower/Makefile                 | 22 ++++++----
 tools/lib/bpf/Makefile                        | 24 +++++++----
 tools/testing/selftests/bpf/Makefile          | 26 ++++++++----
 .../selftests/bpf/test_bpftool_build.sh       |  4 ++
 13 files changed, 171 insertions(+), 77 deletions(-)

Comments

Andrii Nakryiko Oct. 8, 2021, 7:13 p.m. UTC | #1

On Thu, Oct 7, 2021 at 12:44 PM Quentin Monnet <[e-mail]> wrote:
>
> Libbpf is used at several locations in the repository. Most of the time,
> the tools relying on it build the library in its own directory, and include
> the headers from there. This works, but this is not the cleanest approach.
> It generates objects outside of the directory of the tool which is being
> built, and it also increases the risk that developers include a header file
> internal to libbpf, which is not supposed to be exposed to user
> applications.
>
> This set adjusts all involved Makefiles to make sure that libbpf is built
> locally (with respect to the tool's directory or provided build directory),
> and by ensuring that "make install_headers" is run from libbpf's Makefile
> to export user headers properly.
>
> This comes at a cost: given that the libbpf was so far mostly compiled in
> its own directory by the different components using it, compiling it once
> would be enough for all those components. With the new approach, each
> component compiles its own version. To mitigate this cost, efforts were
> made to reuse the compiled library when possible:
>
> - Make the bpftool version in samples/bpf reuse the library previously
>   compiled for the selftests.
> - Make the bpftool version in BPF selftests reuse the library previously
>   compiled for the selftests.
> - Similarly, make resolve_btfids in BPF selftests reuse the same compiled
>   library.
> - Similarly, make runqslower in BPF selftests reuse the same compiled
>   library; and make it rely on the bpftool version also compiled from the
>   selftests (instead of compiling its own version).
> - runqslower, when compiled independently, needs its own version of
>   bpftool: make them share the same compiled libbpf.
>
> As a result:
>
> - Compiling the samples/bpf should compile libbpf just once.
> - Compiling the BPF selftests should compile libbpf just once.
> - Compiling the kernel (with BTF support) should now lead to compiling
>   libbpf twice: one for resolve_btfids, one for kernel/bpf/preload.
> - Compiling runqslower individually should compile libbpf just once. Same
>   thing for bpftool, resolve_btfids, and kernel/bpf/preload/iterators.
>
> (Not accounting for the boostrap version of libbpf required by bpftool,
> which was already placed under a dedicated .../boostrap/libbpf/ directory,
> and for which the count remains unchanged.)
>
> A few commits in the series also contain drive-by clean-up changes for
> bpftool includes, samples/bpf/.gitignore, or test_bpftool_build.sh. Please
> refer to individual commit logs for details.
>
> v4:
>   - Make the "libbpf_hdrs" dependency an order-only dependency in
>     kernel/bpf/preload/Makefile, samples/bpf/Makefile, and
>     tools/bpf/runqslower/Makefile. This is to avoid to unconditionally
>     recompile the targets.
>   - samples/bpf/.gitignore: prefix objects with a "/" to mark that we
>     ignore them when at the root of the samples/bpf/ directory.
>   - libbpf: add a commit to make "install_headers" depend on the header
>     files, to avoid exporting again if the sources are older than the
>     targets. This ensures that applications relying on those headers are
>     not rebuilt unnecessarily.
>   - bpftool: uncouple the copy of nlattr.h from libbpf target, to have it
>     depend on the source header itself. By avoiding to reinstall this
>     header every time, we avoid unnecessary builds of bpftool.
>   - samples/bpf: Add a new commit to remove the FORCE dependency for
>     libbpf, and replace it with a "$(wildcard ...)" on the .c/.h files in
>     libbpf's directory. This is to avoid always recompiling libbpf/bpftool.
>   - Adjust prefixes in commit subjects.
>
> v3:
>   - Remove order-only dependencies on $(LIBBPF_INCLUDE) (or equivalent)
>     directories, given that they are created by libbpf's Makefile.
>   - Add libbpf as a dependency for bpftool/resolve_btfids/runqslower when
>     they are supposed to reuse a libbpf compiled previously. This is to
>     avoid having several libbpf versions being compiled simultaneously in
>     the same directory with parallel builds. Even if this didn't show up
>     during tests, let's remain on the safe side.
>   - kernel/bpf/preload/Makefile: Rename libbpf-hdrs (dash) dependency as
>     libbpf_hdrs.
>   - samples/bpf/.gitignore: Add bpftool/
>   - samples/bpf/Makefile: Change "/bin/rm -rf" to "$(RM) -r".
>   - samples/bpf/Makefile: Add missing slashes for $(LIBBPF_OUTPUT) and
>     $(LIBBPF_DESTDIR) when buildling bpftool
>   - samples/bpf/Makefile: Add a dependency to libbpf's headers for
>     $(TRACE_HELPERS).
>   - bpftool's Makefile: Use $(LIBBPF) instead of equivalent (but longer)
>     $(LIBBPF_OUTPUT)libbpf.a
>   - BPF iterators' Makefile: build bpftool in .output/bpftool (instead of
>     .output/), add and clean up variables.
>   - runqslower's Makefile: Add an explicit dependency on libbpf's headers
>     to several objects. The dependency is not required (libbpf should have
>     been compiled and so the headers exported through other dependencies
>     for those targets), but they better mark the logical dependency and
>     should help if exporting the headers changed in the future.
>   - New commit to add an "install-bin" target to bpftool, to avoid
>     installing bash completion when buildling BPF iterators and selftests.
>
> v2: Declare an additional dependency on libbpf's headers for
>     iterators/iterators.o in kernel/preload/Makefile to make sure that
>     these headers are exported before we compile the object file (and not
>     just before we link it).
>
> Quentin Monnet (12):
>   libbpf: skip re-installing headers file if source is older than target
>   bpftool: remove unused includes to <bpf/bpf_gen_internal.h>
>   bpftool: install libbpf headers instead of including the dir
>   tools/resolve_btfids: install libbpf headers when building
>   tools/runqslower: install libbpf headers when building
>   bpf: preload: install libbpf headers when building
>   bpf: iterators: install libbpf headers when building
>   samples/bpf: update .gitignore
>   samples/bpf: install libbpf headers when building
>   samples/bpf: do not FORCE-recompile libbpf
>   selftests/bpf: better clean up for runqslower in test_bpftool_build.sh
>   bpftool: add install-bin target to install binary only
>
>  kernel/bpf/preload/Makefile                   | 25 ++++++++---
>  kernel/bpf/preload/iterators/Makefile         | 39 +++++++++++------
>  samples/bpf/.gitignore                        |  4 ++
>  samples/bpf/Makefile                          | 42 ++++++++++++++-----
>  tools/bpf/bpftool/Makefile                    | 39 ++++++++++-------
>  tools/bpf/bpftool/gen.c                       |  1 -
>  tools/bpf/bpftool/prog.c                      |  1 -
>  tools/bpf/resolve_btfids/Makefile             | 17 +++++---
>  tools/bpf/resolve_btfids/main.c               |  4 +-
>  tools/bpf/runqslower/Makefile                 | 22 ++++++----
>  tools/lib/bpf/Makefile                        | 24 +++++++----
>  tools/testing/selftests/bpf/Makefile          | 26 ++++++++----
>  .../selftests/bpf/test_bpftool_build.sh       |  4 ++
>  13 files changed, 171 insertions(+), 77 deletions(-)
>
> --
> 2.30.2
>

Tons of ungrateful work, thank you! Applied to bpf-next.

I did a few clean ups (from my POV), see comments on relevant patches.
Also in a bunch of Makefiles I've moved `| $(LIBBPF_OUTPUT)` to the
same line if the line wasn't overly long. 80 characters is not a law,
and I preferred single-line Makefile target definitions, if possible.

There is one problem in bpftool's Makefile, but it works with a
limited case of single file today. Please follow up with a proper fix.

Btw, running make in bpftool's directory, I'm getting:

make[1]: Entering directory '/data/users/andriin/linux/tools/lib/bpf'
make[1]: Entering directory '/data/users/andriin/linux/tools/lib/bpf'
make[1]: Nothing to be done for 'install_headers'.
make[1]: Leaving directory '/data/users/andriin/linux/tools/lib/bpf'
make[1]: Leaving directory '/data/users/andriin/linux/tools/lib/bpf'

Not sure how useful those are, might be better to disable that.

When running libbpf's make, we constantly getting this annoying warning:

Warning: Kernel ABI header at 'tools/include/uapi/linux/netlink.h'
differs from latest version at 'include/uapi/linux/netlink.h'
Warning: Kernel ABI header at 'tools/include/uapi/linux/if_link.h'
differs from latest version at 'include/uapi/linux/if_link.h'

If you will get a chance, maybe you can get rid of that as well? I
don't think we need to stay up to date with netlink.h and if_link.h,
so this seems like just a noise.

There was also

make[4]: Nothing to be done for 'install_headers'.

when building the kernel. It probably is coming from either
bpf_preload or iterators, but maybe also resolve_btfids, I didn't try
to narrow this down. Also seems like a noise, tbh. There are similar
useless notifications when building selftests/bpf. If it doesn't take
too much time to clean all that up, I'd greatly appreciate that!

But really great work, thanks for sticking with it!

Quentin Monnet Oct. 9, 2021, 9:03 p.m. UTC | #2

On Fri, 8 Oct 2021 at 20:13, Andrii Nakryiko <[e-mail]> wrote:

> Tons of ungrateful work, thank you! Applied to bpf-next.
>
> I did a few clean ups (from my POV), see comments on relevant patches.

Thanks for that. I don't mind the clean ups. There are several of them
I considered before sending but wasn't sure about, so it's a good
thing that you did it :).

> Also in a bunch of Makefiles I've moved `| $(LIBBPF_OUTPUT)` to the
> same line if the line wasn't overly long. 80 characters is not a law,
> and I preferred single-line Makefile target definitions, if possible.

No particular preference on my side, so OK.

>
> There is one problem in bpftool's Makefile, but it works with a
> limited case of single file today. Please follow up with a proper fix.

Right, good catch. I'm sending the fix.

>
> Btw, running make in bpftool's directory, I'm getting:
>
> make[1]: Entering directory '/data/users/andriin/linux/tools/lib/bpf'
> make[1]: Entering directory '/data/users/andriin/linux/tools/lib/bpf'
> make[1]: Nothing to be done for 'install_headers'.
> make[1]: Leaving directory '/data/users/andriin/linux/tools/lib/bpf'
> make[1]: Leaving directory '/data/users/andriin/linux/tools/lib/bpf'
>
> Not sure how useful those are, might be better to disable that.

I had a look for bpftool, this is because we always descend into
libbpf's directory (FORCE target). Removing this FORCE target as I did
in samples/bpf/ avoids the descent and clears the output. I'll send a
patch.

>
> When running libbpf's make, we constantly getting this annoying warning:
>
> Warning: Kernel ABI header at 'tools/include/uapi/linux/netlink.h'
> differs from latest version at 'include/uapi/linux/netlink.h'
> Warning: Kernel ABI header at 'tools/include/uapi/linux/if_link.h'
> differs from latest version at 'include/uapi/linux/if_link.h'
>
> If you will get a chance, maybe you can get rid of that as well? I
> don't think we need to stay up to date with netlink.h and if_link.h,
> so this seems like just a noise.

I can look into that. Are you sure you want the warnings removed? Or
would it be cleaner to simply update the headers?

>
> There was also
>
> make[4]: Nothing to be done for 'install_headers'.
>
> when building the kernel. It probably is coming from either
> bpf_preload or iterators, but maybe also resolve_btfids, I didn't try
> to narrow this down. Also seems like a noise, tbh. There are similar
> useless notifications when building selftests/bpf. If it doesn't take
> too much time to clean all that up, I'd greatly appreciate that!

I haven't looked into it yet, but I can do as a follow-up. I'll post
the patches for bpftool first because I prefer to submit the fix for
bpftool's Makefile as soon as possible, and will look at this next.

Thanks,
Quentin

Andrii Nakryiko Oct. 11, 2021, 6:46 a.m. UTC | #3

On Sat, Oct 9, 2021 at 11:03 PM Quentin Monnet <[e-mail]> wrote:
>
> On Fri, 8 Oct 2021 at 20:13, Andrii Nakryiko <[e-mail]> wrote:
>
> > Tons of ungrateful work, thank you! Applied to bpf-next.
> >
> > I did a few clean ups (from my POV), see comments on relevant patches.
>
> Thanks for that. I don't mind the clean ups. There are several of them
> I considered before sending but wasn't sure about, so it's a good
> thing that you did it :).
>
> > Also in a bunch of Makefiles I've moved `| $(LIBBPF_OUTPUT)` to the
> > same line if the line wasn't overly long. 80 characters is not a law,
> > and I preferred single-line Makefile target definitions, if possible.
>
> No particular preference on my side, so OK.
>
> >
> > There is one problem in bpftool's Makefile, but it works with a
> > limited case of single file today. Please follow up with a proper fix.
>
> Right, good catch. I'm sending the fix.

thanks

>
> >
> > Btw, running make in bpftool's directory, I'm getting:
> >
> > make[1]: Entering directory '/data/users/andriin/linux/tools/lib/bpf'
> > make[1]: Entering directory '/data/users/andriin/linux/tools/lib/bpf'
> > make[1]: Nothing to be done for 'install_headers'.
> > make[1]: Leaving directory '/data/users/andriin/linux/tools/lib/bpf'
> > make[1]: Leaving directory '/data/users/andriin/linux/tools/lib/bpf'
> >
> > Not sure how useful those are, might be better to disable that.
>
> I had a look for bpftool, this is because we always descend into
> libbpf's directory (FORCE target). Removing this FORCE target as I did
> in samples/bpf/ avoids the descent and clears the output. I'll send a
> patch.

There is a way to prevent make from logging these enter/leave
messages, which will solve a similar problem discussed below.

>
> >
> > When running libbpf's make, we constantly getting this annoying warning:
> >
> > Warning: Kernel ABI header at 'tools/include/uapi/linux/netlink.h'
> > differs from latest version at 'include/uapi/linux/netlink.h'
> > Warning: Kernel ABI header at 'tools/include/uapi/linux/if_link.h'
> > differs from latest version at 'include/uapi/linux/if_link.h'
> >
> > If you will get a chance, maybe you can get rid of that as well? I
> > don't think we need to stay up to date with netlink.h and if_link.h,
> > so this seems like just a noise.
>
> I can look into that. Are you sure you want the warnings removed? Or
> would it be cleaner to simply update the headers?

Yeah, let's remove checks for those headers. We don't need to keep
them up to date, if there will be new features we need to use from
libbpf, we can update, if it's not already up-to-date.

>
> >
> > There was also
> >
> > make[4]: Nothing to be done for 'install_headers'.
> >
> > when building the kernel. It probably is coming from either
> > bpf_preload or iterators, but maybe also resolve_btfids, I didn't try
> > to narrow this down. Also seems like a noise, tbh. There are similar
> > useless notifications when building selftests/bpf. If it doesn't take
> > too much time to clean all that up, I'd greatly appreciate that!
>
> I haven't looked into it yet, but I can do as a follow-up. I'll post
> the patches for bpftool first because I prefer to submit the fix for
> bpftool's Makefile as soon as possible, and will look at this next.

sure. See also above about just silencing these somewhat useless
messages (it's some make variable or something like that, don't
remember)

>
> Thanks,
> Quentin