On Sun, Dec 15, 2019 at 8:42 PM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> 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@xxxxxxxxx> 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.
