Re: [PATCH bpf-next 2/6] libbpf: move bpf_helpers.h, bpf_endian.h into libbpf

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

On Tue, Oct 1, 2019 at 3:45 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On Mon, Sep 30, 2019 at 11:58:51AM -0700, Andrii Nakryiko wrote:
> > Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they
> > are installed along the other libbpf headers.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> [...]
> > new file mode 100644
> > index 000000000000..fbe28008450f
> > --- /dev/null
> > +++ b/tools/lib/bpf/bpf_endian.h
> > @@ -0,0 +1,72 @@
> > +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> > +#ifndef __BPF_ENDIAN__
> > +#define __BPF_ENDIAN__
> > +

[...]

> > +#define bpf_cpu_to_be64(x)                   \
> > +     (__builtin_constant_p(x) ?              \
> > +      __bpf_constant_cpu_to_be64(x) : __bpf_cpu_to_be64(x))
> > +#define bpf_be64_to_cpu(x)                   \
> > +     (__builtin_constant_p(x) ?              \
> > +      __bpf_constant_be64_to_cpu(x) : __bpf_be64_to_cpu(x))
>
> Nit: if we move this into a public API for libbpf, we should probably
> also define for sake of consistency a bpf_cpu_to_be{64,32,16}() and
> bpf_be{64,32,16}_to_cpu() and have the latter two point to the existing
> bpf_hton{l,s}() and bpf_ntoh{l,s}() macros.

I think these deserve better tests before we add more stuff, both from
BPF-side and userland-side (as they are supposed to be includeable
from both, right)? I'm hesitant adding new unfamiliar macro/builtins
without tests, but I don't want to get distracted with that right now,
especially this patch set already becomes bigger than I'd hope.

Given we are talking about adding new stuff which is not breaking
change, we can add it later after we move this as is, agree?

>
> > +#endif /* __BPF_ENDIAN__ */
>
> > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > new file mode 100644
> > index 000000000000..a1d9b97b8e15
> > --- /dev/null
> > +++ b/tools/lib/bpf/bpf_helpers.h
> > @@ -0,0 +1,527 @@
> > +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> > +#ifndef __BPF_HELPERS__
> > +#define __BPF_HELPERS__
> > +
> > +#define __uint(name, val) int (*name)[val]
> > +#define __type(name, val) val *name
> > +
> > +/* helper macro to print out debug messages */
> > +#define bpf_printk(fmt, ...)                         \
> > +({                                                   \
> > +     char ____fmt[] = fmt;                           \
> > +     bpf_trace_printk(____fmt, sizeof(____fmt),      \
> > +                      ##__VA_ARGS__);                \
> > +})
>
> Did you double check if by now via .rodata we can have the fmt as
> const char * and add __attribute__ ((format (printf, 1, 2))) to it?
> If that works we should avoid having to copy the string as above in
> the API.

This doesn't work still, unfortunately. Eventually, though, we'll be
able to essentially nop it out with direct call to bpf_trace_printk,
so I'm not concerned about future API burden.

>
> > +/* helper macro to place programs, maps, license in
> > + * different sections in elf_bpf file. Section names
> > + * are interpreted by elf_bpf loader
> > + */
> > +#define SEC(NAME) __attribute__((section(NAME), used))
> > +
> > +/* helper functions called from eBPF programs written in C */
>
> As mentioned earlier, the whole helper function description below
> should get a big cleanup in here when moved into libbpf API.

Right, I just recalled that today, sorry I didn't do it for this version.

There were two things you mentioned on that thread that you wanted to clean up:
1. using __u32 instead int and stuff like that
2. using macro to hide some of the ugliness of (void *) = BPF_FUNC_xxx

So with 1) I'm concerned that we can't just assume that __u32 is
always going to be defined. Also we need bpf_helpers.h to be usable
both with including system headers, as well as auto-generated
vmlinux.h. In first case, I don't think we can assume that typedef is
always defined, in latter case we can't really define it on our own.
So I think we should just keep it as int, unsigned long long, etc.
Thoughts?

For 2), I'm doing that right now, but it's not that much cleaner, let's see.

Am I missing something else?

>
> > +static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
> > +     (void *) BPF_FUNC_map_lookup_elem;
> > +static int (*bpf_map_update_elem)(void *map, const void *key, const void *value,
> > +                               unsigned long long flags) =
> [...]
> > +
> > +/* llvm builtin functions that eBPF C program may use to
> > + * emit BPF_LD_ABS and BPF_LD_IND instructions
> > + */
> > +struct sk_buff;
> > +unsigned long long load_byte(void *skb,
> > +                          unsigned long long off) asm("llvm.bpf.load.byte");
> > +unsigned long long load_half(void *skb,
> > +                          unsigned long long off) asm("llvm.bpf.load.half");
> > +unsigned long long load_word(void *skb,
> > +                          unsigned long long off) asm("llvm.bpf.load.word");
>
> These should be removed from the public API, no point in adding legacy/
> deprecated stuff in there.

Oh, cool, never knew what that stuff is anyway :)

>
> > +/* a helper structure used by eBPF C program
> > + * to describe map attributes to elf_bpf loader
> > + */
> > +struct bpf_map_def {
> > +     unsigned int type;
> > +     unsigned int key_size;
> > +     unsigned int value_size;
> > +     unsigned int max_entries;
> > +     unsigned int map_flags;
> > +     unsigned int inner_map_idx;
> > +     unsigned int numa_node;
> > +};
> > +
> > +#define BPF_ANNOTATE_KV_PAIR(name, type_key, type_val)               \
> > +     struct ____btf_map_##name {                             \
> > +             type_key key;                                   \
> > +             type_val value;                                 \
> > +     };                                                      \
> > +     struct ____btf_map_##name                               \
> > +     __attribute__ ((section(".maps." #name), used))         \
> > +             ____btf_map_##name = { }
>
> Same here.

We still use it in few selftests, I'll move it into selftests-only header.

>
> > +static int (*bpf_skb_load_bytes)(void *ctx, int off, void *to, int len) =
> > +     (void *) BPF_FUNC_skb_load_bytes;
> [...]
>
> Given we already move bpf_endian.h to a separate header, I'd do the
> same for all tracing specifics as well, e.g. bpf_tracing.h.

You mean all the stuff below, right? I'll extract it into separate
header, no problem.

What about CO-RE stuff. It's not strictly for tracing, so does it make
sense to keep it here?

>
> > +/* Scan the ARCH passed in from ARCH env variable (see Makefile) */
> > +#if defined(__TARGET_ARCH_x86)
> > +     #define bpf_target_x86
> > +     #define bpf_target_defined
> > +#elif defined(__TARGET_ARCH_s390)

[...]

> > --
> > 2.17.1
> >