Re: [PATCH bpf-next 4/6] libbpf: add BPF_CORE_READ/BPF_CORE_READ_INTO helpers

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Oct 1, 2019 at 2:46 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Oct 1, 2019, at 2:25 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Oct 1, 2019 at 2:14 PM Song Liu <songliubraving@fb.com> wrote:
> >>
> >>
> >>> On Sep 30, 2019, at 11:58 AM, Andrii Nakryiko <andriin@fb.com> 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@fb.com>
> >>> ---
> >>> 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 :)

>
> > Prefixing with _libbpf is an option, but it will make it super ugly
> > and hard to follow (I've spent a bunch of time to even get it to the
> > current state), so I'd like to avoid that.
>
> BPF programs will not use these marcos directly, so I feel it is OK to
> pay the pain of _libbpf prefix, as it is contained within this file.

I disagree, having more convoluted multi-line macros will eventually
lead to a stupid mistake or typo, which could have been spotted
otherwise. Plus, if user screws up (which is inevitable anyway) usage
of these macro, he will be presented with long and incomprehensible
chain of macro with more obscure names than necessary. If you think
that internal names don't matter, ask anyone who had to decipher
compilation errors involving C++ standard library templates. I'd be OK
with that if there was a real risk of name conflict, but I disagree
with the premise.

>
> Thanks,
> Song
>
>
>