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.
