Message-ID: <20191216044253.6stsb7nsxf35cujl@ast-mbp.dhcp.thefacebook.com>
Date:   Sun, 15 Dec 2019 20:42:55 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...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 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.

Powered by blists - more mailing lists