Re: [PATCH bpf-next v2 4/6] bpf, libbpf: add bpf_tail_call_static helper for bpf programs

On 9/28/20 7:39 PM, Andrii Nakryiko wrote:

On Mon, Sep 28, 2020 at 7:39 AM Daniel Borkmann [off-list ref] wrote:

quoted

Port of tail_call_static() helper function from Cilium's BPF code base [0]
to libbpf, so others can easily consume it as well. We've been using this
in production code for some time now. The main idea is that we guarantee
that the kernel's BPF infrastructure and JIT (here: x86_64) can patch the
JITed BPF insns with direct jumps instead of having to fall back to using
expensive retpolines. By using inline asm, we guarantee that the compiler
won't merge the call from different paths with potentially different
content of r2/r3.

We're also using Cilium's __throw_build_bug() macro (here as: __bpf_unreachable())
in different places as a neat trick to trigger compilation errors when
compiler does not remove code at compilation time. This works for the BPF
back end as it does not implement the __builtin_trap().

   [0] https://github.com/cilium/cilium/commit/f5537c26020d5297b70936c6b7d03a1e412a1035

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Andrii Nakryiko <redacted>
---

few optional nits below, but looks good to me:

Acked-by: Andrii Nakryiko <redacted>

[...]

Thanks!

quoted

+/*
+ * Helper function to perform a tail call with a constant/immediate map slot.
+ */
+static __always_inline void
+bpf_tail_call_static(void *ctx, const void *map, const __u32 slot)

nit: const void *ctx would work here, right? would avoid users having
to do unnecessary casts in some cases

The bpf_tail_call() UAPI signature only has 'void *ctx' as well, so shouldn't
cause any issues as drop-in replacement wrt casts, but also ctx here is not always
a read-only [context] object either so it may potentially be a bit misleading to
users to convert it to const.