On Tue, Oct 1, 2019 at 4:44 PM Song Liu <songliubraving@xxxxxx> wrote: > > > > > On Oct 1, 2019, at 3:42 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > > > On Tue, Oct 1, 2019 at 2:46 PM Song Liu <songliubraving@xxxxxx> wrote: > >> > >> > >> > >>> On Oct 1, 2019, at 2:25 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > >>> > >>> On Tue, Oct 1, 2019 at 2:14 PM Song Liu <songliubraving@xxxxxx> wrote: > >>>> > >>>> > >>>>> On Sep 30, 2019, at 11:58 AM, Andrii Nakryiko <andriin@xxxxxx> wrote: > >>>>> > >>>>> Add few macros simplifying BCC-like multi-level probe reads, while also > >>>>> emitting CO-RE relocations for each read. > >>>>> > >>>>> Signed-off-by: Andrii Nakryiko <andriin@xxxxxx> > >>>>> --- > >>>>> tools/lib/bpf/bpf_helpers.h | 151 +++++++++++++++++++++++++++++++++++- > >>>>> 1 file changed, 147 insertions(+), 4 deletions(-) > >>>>> > >>>>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > >>>>> index a1d9b97b8e15..51e7b11d53e8 100644 > >>>>> --- a/tools/lib/bpf/bpf_helpers.h > >>>>> +++ b/tools/lib/bpf/bpf_helpers.h > >>>>> @@ -19,6 +19,10 @@ > >>>>> */ > >>>>> #define SEC(NAME) __attribute__((section(NAME), used)) > >>>>> > >>>>> +#ifndef __always_inline > >>>>> +#define __always_inline __attribute__((always_inline)) > >>>>> +#endif > >>>>> + > >>>>> /* helper functions called from eBPF programs written in C */ > >>>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = > >>>>> (void *) BPF_FUNC_map_lookup_elem; > >>>>> @@ -505,7 +509,7 @@ struct pt_regs; > >>>>> #endif > >>>>> > >>>>> /* > >>>>> - * BPF_CORE_READ abstracts away bpf_probe_read() call and captures offset > >>>>> + * bpf_core_read() abstracts away bpf_probe_read() call and captures field > >>>>> * relocation for source address using __builtin_preserve_access_index() > >>>>> * built-in, provided by Clang. > >>>>> * > >>>>> @@ -520,8 +524,147 @@ struct pt_regs; > >>>>> * actual field offset, based on target kernel BTF type that matches original > >>>>> * (local) BTF, used to record relocation. > >>>>> */ > >>>>> -#define BPF_CORE_READ(dst, src) \ > >>>>> - bpf_probe_read((dst), sizeof(*(src)), \ > >>>>> - __builtin_preserve_access_index(src)) > >>>>> +#define bpf_core_read(dst, sz, src) \ > >>>>> + bpf_probe_read(dst, sz, \ > >>>>> + (const void *)__builtin_preserve_access_index(src)) > >>>>> + > >>>>> +/* > >>>>> + * bpf_core_read_str() is a thin wrapper around bpf_probe_read_str() > >>>>> + * additionally emitting BPF CO-RE field relocation for specified source > >>>>> + * argument. > >>>>> + */ > >>>>> +#define bpf_core_read_str(dst, sz, src) \ > >>>>> + bpf_probe_read_str(dst, sz, \ > >>>>> + (const void *)__builtin_preserve_access_index(src)) > >>>>> + > >>>>> +#define ___concat(a, b) a ## b > >>>>> +#define ___apply(fn, n) ___concat(fn, n) > >>>>> +#define ___nth(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, __11, N, ...) N > >>>> > >>>> We are adding many marcos with simple names: ___apply(), ___nth. So I worry > >>>> they may conflict with macro definitions from other libraries. Shall we hide > >>>> them in .c files or prefix/postfix them with _libbpf or something? > >>> > >>> Keep in mind, this is the header that's included from BPF code. > >>> > >>> They are prefixed with three underscores, I was hoping it's good > >>> enough to avoid accidental conflicts. It's unlikely someone will have > >>> macros with the same names **in BPF-side code**. > >> > >> BPF side code would include kernel headers. So there are many headers > >> to conflict with. And we won't know until somebody want to trace certain > >> kernel structure. > > > > We have all the kernel sources at our disposal, there's no need to > > guess :) There is no instance of ___apply, ___concat, ___nth, > > ___arrow, ___last, ___nolast, or ___type, not even speaking about > > other more specific names. There are currently two instances of > > "____last_____" used in a string. And I'm certainly not afraid that > > user code can use triple-underscored identifier with exact those names > > and complain about bpf_helpers.h :) > > I worry more about _future_ conflicts, that someone may add ___apply to You can say the same about pretty much any name that user might use, that's just the fact of life with C language without namespaces. I don't think that justifies usage of obscure names. Look at SEC macro, for instance. It's also an enum value in drivers/sbus/char/oradax.c, but it might some day end up in one of driver's headers. This is probably not a reason to rename it, though. > some kernel header file and break some BPF programs. Since these BPF > programs are not in-tree, it is very difficult to test them properly. > We have had name conflicts from other libraries, so I hope we don't create > more ourselves. Let's agree to come back to this problem when and if we ever encounter it. All those ___xxx macro are internal and users shouldn't rely on them, which means if we ever get a real conflict, we'll be able to rename them to avoid the conflict. > > Thanks, > Song
