bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Jakub Kicinski <jakub.kicinski@netronome.com>
Cc: David Ahern <dsahern@gmail.com>, 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>, Jiri Benc <jbenc@redhat.com>
Subject: Re: [PATCH v3 bpf-next 5/7] libbpf: move bpf_{helpers,endian,tracing}.h into libbpf
Date: Fri, 4 Oct 2019 13:21:55 -0700	[thread overview]
Message-ID: <CAEf4Bzbw-=NSKMYpDcTY1Pw9NfeRJ5+KpScWg4wHfoDG18dKPQ@mail.gmail.com> (raw)
In-Reply-To: <20191004113026.4c23cd41@cakuba.hsd1.ca.comcast.net>

On Fri, Oct 4, 2019 at 11:30 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> On Fri, 4 Oct 2019 09:00:42 -0700, Andrii Nakryiko wrote:
> > On Fri, Oct 4, 2019 at 8:44 AM David Ahern <dsahern@gmail.com> wrote:
> >> > 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.
> Surely for distroes tho - they would have kernel headers matching the
> kernel release they ship. If parts of libbpf from GH only work with
> the latest kernel, distroes should ship libbpf from the kernel source,
> rather than GH.
> > > > 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.
> Having a header which works today, but may not work tomorrow is going
> to be pretty bad user experience :( No matter how many warnings you put
> in the source people will get caught off guard by this :(
> If you define the current state as "users can use all features of
> libbpf and nothing should break on libbpf update" (which is in my
> understanding a goal of the project, we bent over backwards trying
> to not break things) then adding this header will in fact make things
> worse. The statement in quotes would no longer be true, no?

So there are few things here.

1. About "adding bpf_helpers.h will make things worse". I
categorically disagree, bpf_helpers.h doesn't exist in user land at
all and it's sorely missing. So adding it is strictly better
experience already. Right now people have to re-declare those helper
signatures and do all kinds of unnecessary hackery just to be able to
use BPF stuff, and they still can run into the same problem with
having too old kernel headers.

Also, I think we should have informal notion of "experimental"
features and APIs which we add to get real-world experience of using
it, before we crystalize it to something that we have to maintain
backwards compatibility and never-breaking user experience for. Its
sometimes impossible to finesse all the tiny issues before we get
prolonged enough experience w/ real applications in a real set up. If
we are going to wait to resolve all the tiny possible problems, we'll
cripple outselves very significantly. I think bpf_helpers.h move walls
into this "experimental" thing, which we obviously will try to
"stabilize" ASAP, but we need it to be part of libbpf first to start
using it in production. So in that sense it's a huge improvement
already and I think we should land it as is and keep improving it, not
stall progress right now.

2. As to the problem of running bleeding-edge libbpf against older
kernel. There are few possible solutions:

a. we hard-code all those BPF_FUNC_ constants. Super painful and not
nice, but will work.

b. copy/paste enum bpf_func_id definition into bpf_helpers.h itself
and try to keep it in sync with UAPI. Apart from obvious redundancy
that involves, we also will need to make sure this doesn't conflict
with vmlinux.h, so enum name should be different and each value should
be different (which means some wort of additional prefix or suffix).

c. BPF UAPI header has __BPF_FUNC_MAPPER macro "iterating" over all
defined values for a particular kernel version. We can use that and
additional macro trickery to conditionally define helpers. Again, we
need to keep in mind that w/ vmlinux.h there is no such macro, so this
should work as well.

I'm happy to hear opinions about these choices (maybe there are some
other I missed), but in any case I'd like to do it in a follow up
patch and land this one as is. It has already quite a lot packed in
it. I personally lean towards c) as it will have a benefit of not
declaring helpers that are not supported by kernel we are compiling
against, even though it requires additional macro trickery.


  parent reply	other threads:[~2019-10-04 20:22 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
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 [this message]
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:

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

  git send-email \
    --in-reply-to='CAEf4Bzbw-=NSKMYpDcTY1Pw9NfeRJ5+KpScWg4wHfoDG18dKPQ@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=jakub.kicinski@netronome.com \
    --cc=jbenc@redhat.com \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \


* 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).