bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: David Ahern <dsahern@gmail.com>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v3 bpf-next 5/7] libbpf: move bpf_{helpers,endian,tracing}.h into libbpf
Date: Fri, 4 Oct 2019 09:00:42 -0700	[thread overview]
Message-ID: <CAEf4BzZr9cxt=JrGYPUhDTRfbBocM18tFFaP+LiJSCF-g4hs2w@mail.gmail.com> (raw)
In-Reply-To: <4fcbe7bf-201a-727a-a6f1-2088aea82a33@gmail.com>

On Fri, Oct 4, 2019 at 8:44 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 10/4/19 9:27 AM, Andrii Nakryiko wrote:
> > On Fri, Oct 4, 2019 at 7:47 AM David Ahern <dsahern@gmail.com> wrote:
> >>
> >> On 10/3/19 3:28 PM, Andrii Nakryiko wrote:
> >>> Move bpf_helpers.h, bpf_tracing.h, and bpf_endian.h into libbpf. Ensure
> >>> they are installed along the other libbpf headers. Also, adjust
> >>> selftests and samples include path to include libbpf now.
> >>
> >> There are side effects to bringing bpf_helpers.h into libbpf if this
> >> gets propagated to the github sync.
> >>
> >> bpf_helpers.h references BPF_FUNC_* which are defined in the
> >> uapi/linux/bpf.h header. That is a kernel version dependent api file
> >> which means attempts to use newer libbpf with older kernel headers is
> >> going to throw errors when compiling bpf programs -- bpf_helpers.h will
> >> contain undefined BPF_FUNC references.
> >
> > That's true, but I'm wondering if maintaining a copy of that enum in
> > bpf_helpers.h itself is a good answer here?
> >
> > bpf_helpers.h will be most probably used with BPF CO-RE and
> > auto-generated vmlinux.h with all the enums and types. In that case,
> > you'll probably want to use vmlinux.h for one of the latest kernels
> > anyways.
>
> I'm not following you; my interpretation of your comment seems like you
> are making huge assumptions.
>
> I build bpf programs for specific kernel versions using the devel
> packages for the specific kernel of interest.

Sure, and you can keep doing that, just don't include bpf_helpers.h?

What I was saying, though, especially having in mind tracing BPF
programs that need to inspect kernel structures, is that it's quite
impractical to have to build many different versions of BPF programs
for each supported kernel version and distribute them in binary form.
So people usually use BCC and do compilation on-the-fly using BCC's
embedded Clang.

BPF CO-RE is providing an alternative, which will allow to pre-compile
your program once for many different kernels you might be running your
program on. There is tooling that eliminates the need for system
headers. Instead we pre-generate a single vmlinux.h header with all
the types/enums/etc, that are then used w/ BPF CO-RE to build portable
BPF programs capable of working on multiple kernel versions.

So what I was pointing out there was that this vmlinux.h would be
ideally generated from latest kernel and not having latest
BPF_FUNC_xxx shouldn't be a problem. But see below about situation
being worse.

>
> >
> > Nevertheless, it is a problem and thanks for bringing it up! I'd say
> > for now we should still go ahead with this move and try to solve with
> > issue once bpf_helpers.h is in libbpf. If bpf_helpers.h doesn't work
> > for someone, it's no worse than it is today when users don't have
> > bpf_helpers.h at all.
> >
>
> If this syncs to the github libbpf, it will be worse than today in the
> sense of compile failures if someone's header file ordering picks
> libbpf's bpf_helpers.h over whatever they are using today.

Today bpf_helpers.h don't exist for users or am I missing something?
bpf_helpers.h right now are purely for selftests. But they are really
useful outside that context, so I'm making it available for everyone
by distributing with libbpf sources. If bpf_helpers.h doesn't work for
some specific use case, just don't use it (yet?).

I'm still failing to see how it's worse than situation today.

  reply	other threads:[~2019-10-04 16:00 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-03 21:28 [PATCH v3 bpf-next 0/7] Move bpf_helpers and add BPF_CORE_READ macros Andrii Nakryiko
2019-10-03 21:28 ` [PATCH v3 bpf-next 1/7] selftests/bpf: undo GCC-specific bpf_helpers.h changes Andrii Nakryiko
2019-10-04  7:00   ` Toke Høiland-Jørgensen
2019-10-03 21:28 ` [PATCH v3 bpf-next 2/7] selftests/bpf: samples/bpf: split off legacy stuff from bpf_helpers.h Andrii Nakryiko
2019-10-04  7:00   ` Toke Høiland-Jørgensen
2019-10-03 21:28 ` [PATCH v3 bpf-next 3/7] selftests/bpf: adjust CO-RE reloc tests for new bpf_core_read() macro Andrii Nakryiko
2019-10-03 21:28 ` [PATCH v3 bpf-next 4/7] selftests/bpf: split off tracing-only helpers into bpf_tracing.h Andrii Nakryiko
2019-10-04  7:01   ` Toke Høiland-Jørgensen
2019-10-03 21:28 ` [PATCH v3 bpf-next 5/7] libbpf: move bpf_{helpers,endian,tracing}.h into libbpf Andrii Nakryiko
2019-10-04  7:01   ` Toke Høiland-Jørgensen
2019-10-04 14:47   ` David Ahern
2019-10-04 15:27     ` Andrii Nakryiko
2019-10-04 15:44       ` David Ahern
2019-10-04 16:00         ` Andrii Nakryiko [this message]
2019-10-04 18:30           ` Jakub Kicinski
2019-10-04 18:37             ` Yonghong Song
2019-10-04 21:04               ` Jakub Kicinski
2019-10-08 15:37               ` Jiri Benc
2019-10-08 18:02                 ` Andrii Nakryiko
2019-10-04 20:21             ` Andrii Nakryiko
2019-10-04 21:06               ` Daniel Borkmann
2019-10-04 21:58                 ` Daniel Borkmann
2019-10-04 22:47                   ` Andrii Nakryiko
2019-10-04 22:51                     ` Andrii Nakryiko
2019-10-04 23:25                       ` Andrii Nakryiko
2019-10-08 15:29             ` Jiri Benc
2019-10-03 21:28 ` [PATCH v3 bpf-next 6/7] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers Andrii Nakryiko
2019-10-03 21:28 ` [PATCH v3 bpf-next 7/7] selftests/bpf: add BPF_CORE_READ and BPF_CORE_READ_STR_INTO macro tests Andrii Nakryiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEf4BzZr9cxt=JrGYPUhDTRfbBocM18tFFaP+LiJSCF-g4hs2w@mail.gmail.com' \
    --to=andrii.nakryiko@gmail.com \
    --cc=andriin@fb.com \
    --cc=ast@fb.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=dsahern@gmail.com \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).