Message-ID: <CAEf4BzbxkRLGENX9zPXKpmWo5Rc-QHkic=t0Cy4TCeBamCD7Qw@mail.gmail.com>
Date:   Mon, 16 Dec 2019 11:34:25 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v4 bpf-next 0/4] Add libbpf-provided extern variables support

On Sun, Dec 15, 2019 at 8:42 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Sun, Dec 15, 2019 at 05:47:01PM -0800, Andrii Nakryiko wrote:
> > On Sun, Dec 15, 2019 at 4:52 PM Alexei Starovoitov
> > <alexei.starovoitov@...il.com> wrote:
> > >
> > > On Fri, Dec 13, 2019 at 05:47:06PM -0800, Andrii Nakryiko wrote:
> > > > It's often important for BPF program to know kernel version or some specific
> > > > config values (e.g., CONFIG_HZ to convert jiffies to seconds) and change or
> > > > adjust program logic based on their values. As of today, any such need has to
> > > > be resolved by recompiling BPF program for specific kernel and kernel
> > > > configuration. In practice this is usually achieved by using BCC and its
> > > > embedded LLVM/Clang. With such set up #ifdef CONFIG_XXX and similar
> > > > compile-time constructs allow to deal with kernel varieties.
> > > >
> > > > With CO-RE (Compile Once – Run Everywhere) approach, this is not an option,
> > > > unfortunately. All such logic variations have to be done as a normal
> > > > C language constructs (i.e., if/else, variables, etc), not a preprocessor
> > > > directives. This patch series add support for such advanced scenarios through
> > > > C extern variables. These extern variables will be recognized by libbpf and
> > > > supplied through extra .extern internal map, similarly to global data. This
> > > > .extern map is read-only, which allows BPF verifier to track its content
> > > > precisely as constants. That gives an opportunity to have pre-compiled BPF
> > > > program, which can potentially use BPF functionality (e.g., BPF helpers) or
> > > > kernel features (types, fields, etc), that are available only on a subset of
> > > > targeted kernels, while effectively eleminating (through verifier's dead code
> > > > detection) such unsupported functionality for other kernels (typically, older
> > > > versions). Patch #3 explicitly tests a scenario of using unsupported BPF
> > > > helper, to validate the approach.
> > > >
> > > > This patch set heavily relies on BTF type information emitted by compiler for
> > > > each extern variable declaration. Based on specific types, libbpf does strict
> > > > checks of config data values correctness. See patch #1 for details.
> > > >
> > > > Outline of the patch set:
> > > > - patch #1 does a small clean up of internal map names contants;
> > > > - patch #2 adds all of the libbpf internal machinery for externs support,
> > > >   including setting up BTF information for .extern data section;
> > > > - patch #3 adds support for .extern into BPF skeleton;
> > > > - patch #4 adds externs selftests, as well as enhances test_skeleton.c test to
> > > >   validate mmap()-ed .extern datasection functionality.
> > >
> > > Applied. Thanks.
> >
> > Great, thanks!
> >
> > >
> > > Looking at the tests that do mkstemp()+write() just to pass a file path
> > > as .kconfig_path option into bpf_object_open_opts() it feels that file only
> > > support for externs is unnecessary limiting. I think it will simplify
> >
> > yeah, it was a bit painful :)
> >
> > > tests and will make the whole extern support more flexible if in addition to
> > > kconfig_path bpf_object_open_opts() would support in-memory configuration.
> >
> > I wanted to keep it simple for users, in case libbpf can't find config
> > file, to just specify its location. But given your feedback here, and
> > you mentioned previously that it would be nice to allow users to
> > specify custom kconfig-like configuration to be exposed as externs as
> > well, how about replacing .kconfig_path, which is a patch to config
> > file, with just .kconfig, which is the **contents** of config file.
> > That way we can support all of the above scenarios, if maybe sometime
> > with a tiny bit of extra work for users:
> >
> > 1. Override real kconfig with custom config (e.g., for testing
> > purposes) - just specify alternative contents.
> > 2. Extend kconfig with some extra configuration - user will have to
> > read real kconfig, then append (or prepend, doesn't matter) custom
> > contents.
> >
> > What I want to avoid is having multiple ways to do this, having to
> > decide whether to augment real Kconfig or completely override it, etc.
> > So one string-based config override is preferable for simplicity.
> > Agreed?
>
> I think user experience would be better if users don't have to know that
> /proc/config.gz exists and even more so if they don't need to read it. By
> default libbpf should pick all CONFIG_* from known locations and in addition if
> extra text is specified for bpf_object_open_opts() the libbpf can take the
> values from there. So may be .kconfig_path can be replaced with
> .additional_key_value_pairs ? I think override of /proc/config.gz is
> unnecessary. Whereas additional predefined externs are useful for testing and
> passing load-time configuration to bpf progs. Like IP addresses, etc.

Yes, agree about usability issues w/ user having to read and unzip
/proc/config.gz. I will add kconfig (const char*) to mean
additions/overrides to Kconfig, which applications could use to
override and augment Kconfig options for their needs. I'd still like
to keep the convention of CONFIG_ and reserve everything else for
static/dynamic linking use case (and kernel-provided externs, as a
special case of dynamic linking). Makes sense?

Daniel expressed concern about opting out of Kconfig parsing
altogether, so as a way to do this, I can keep kconfig_path option
anyways, and define that empty string means "no Kconfig parsing". But
let's see if Daniel still thinks it a problem.

Powered by blists - more mailing lists