From: Daniel Borkmann <daniel@iogearbox.net>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
john fastabend <john.fastabend@gmail.com>,
Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next 4/6] bpf, libbpf: add bpf_tail_call_static helper for bpf programs
Date: Fri, 25 Sep 2020 21:52:27 +0200 [thread overview]
Message-ID: <bdddb94c-555c-3833-f82e-a53f83b1a77e@iogearbox.net> (raw)
In-Reply-To: <CAEf4BzZmpLOCSp4wvXWHzmfZHq5R4S32M0_V5OvGA+QQGGG43w@mail.gmail.com>
On 9/25/20 6:50 PM, Andrii Nakryiko wrote:
> On Fri, Sep 25, 2020 at 8:52 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 9/25/20 5:42 PM, Daniel Borkmann wrote:
>>> On 9/25/20 12:17 AM, Daniel Borkmann wrote:
>>>> On 9/24/20 10:53 PM, Andrii Nakryiko wrote:
>>>>> On Thu, Sep 24, 2020 at 11:22 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>>>>
>>>>>> 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 __throw_build_bug() macro 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 backend as it does not implement
>>>>>> the __builtin_trap().
>>>>>>
>>>>>> [0] https://github.com/cilium/cilium/commit/f5537c26020d5297b70936c6b7d03a1e412a1035
>>>>>>
>>>>>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>>>>>> ---
>>>>>> tools/lib/bpf/bpf_helpers.h | 32 ++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 32 insertions(+)
>>>>>>
>>>>>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
>>>>>> index 1106777df00b..18b75a4c82e6 100644
>>>>>> --- a/tools/lib/bpf/bpf_helpers.h
>>>>>> +++ b/tools/lib/bpf/bpf_helpers.h
>>>>>> @@ -53,6 +53,38 @@
>>>>>> })
>>>>>> #endif
>>>>>>
>>>>>> +/*
>>>>>> + * Misc useful helper macros
>>>>>> + */
>>>>>> +#ifndef __throw_build_bug
>>>>>> +# define __throw_build_bug() __builtin_trap()
>>>>>> +#endif
>>>>>
>>>>> this will become part of libbpf stable API, do we want/need to expose
>>>>> it? If we want to expose it, then we should probably provide a better
>>>>> description.
>>>>>
>>>>> But also curious, how is it better than _Static_assert() (see
>>>>> test_cls_redirect.c), which also allows to provide a better error
>>>>> message?
>>>>
>>>> Need to get back to you whether that has same semantics. We use the __throw_build_bug()
>>>> also in __bpf_memzero() and friends [0] as a way to trigger a hard build bug if we hit
>>>> a default switch-case [0], so we detect unsupported sizes which are not covered by the
>>>> implementation yet. If _Static_assert (0, "foo") does the trick, we could also use that;
>>>> will check with our code base.
>>>
>>> So _Static_assert() won't work here, for example consider:
>>>
>>> # cat f1.c
>>> int main(void)
>>> {
>>> if (0)
>>> _Static_assert(0, "foo");
>>> return 0;
>>> }
>>> # clang -target bpf -Wall -O2 -c f1.c -o f1.o
>>> f1.c:4:3: error: expected expression
>>> _Static_assert(0, "foo");
>>> ^
>>> 1 error generated.
>>
>> .. aaand it looks like I need some more coffee. ;-) But result is the same after all:
>>
>> # clang -target bpf -Wall -O2 -c f1.c -o f1.o
>> f1.c:4:3: error: static_assert failed "foo"
>> _Static_assert(0, "foo");
>> ^ ~
>> 1 error generated.
>>
>> # cat f1.c
>> int main(void)
>> {
>> if (0) {
>> _Static_assert(0, "foo");
>> }
>> return 0;
>> }
>
> You need still more :-P. For you use case it will look like this:
>
> $ cat test-bla.c
> int bar(int x) {
> _Static_assert(!__builtin_constant_p(x), "not a constant!");
> return x;
> }
>
> int foo() {
> bar(123);
> return 0;
> }
> $ clang -target bpf -O2 -c test-bla.c -o test-bla.o
> $ echo $?
> 0
Right, but that won't work for example for the use case to detect switch cases which fall
into default case as mentioned with the mem* optimizations earlier in this thread.
> But in general to ensure unreachable code it's probably useful anyway
> to have this. How about calling it __bpf_build_error() or maybe even
> __bpf_unreachable()?
I think the __bpf_unreachable() sounds best to me, will use that.
>>> In order for it to work as required form the use-case, the _Static_assert() must not trigger
>>> here given the path is unreachable and will be optimized away. I'll add a comment to the
>>> __throw_build_bug() helper. Given libbpf we should probably also prefix with bpf_. If you see
>>> a better name that would fit, pls let me know.
>>>
>>>> [0] https://github.com/cilium/cilium/blob/master/bpf/include/bpf/builtins.h
>>> Thanks,
>>> Daniel
>>
next prev parent reply other threads:[~2020-09-25 20:36 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-24 18:21 [PATCH bpf-next 0/6] Various BPF helper improvements Daniel Borkmann
2020-09-24 18:21 ` [PATCH bpf-next 1/6] bpf: add classid helper only based on skb->sk Daniel Borkmann
2020-09-25 14:46 ` Jakub Kicinski
2020-09-25 15:35 ` Daniel Borkmann
2020-09-24 18:21 ` [PATCH bpf-next 2/6] bpf, net: rework cookie generator as per-cpu one Daniel Borkmann
2020-09-24 18:58 ` Eric Dumazet
2020-09-24 22:03 ` Daniel Borkmann
2020-09-25 7:49 ` Eric Dumazet
2020-09-25 9:26 ` Daniel Borkmann
2020-09-25 15:00 ` Jakub Kicinski
2020-09-25 15:15 ` Eric Dumazet
2020-09-25 15:31 ` Jakub Kicinski
2020-09-25 15:45 ` Eric Dumazet
2020-09-24 18:21 ` [PATCH bpf-next 3/6] bpf: add redirect_neigh helper as redirect drop-in Daniel Borkmann
2020-09-24 22:12 ` David Ahern
2020-09-24 22:19 ` Daniel Borkmann
2020-09-24 18:21 ` [PATCH bpf-next 4/6] bpf, libbpf: add bpf_tail_call_static helper for bpf programs Daniel Borkmann
2020-09-24 20:53 ` Andrii Nakryiko
2020-09-24 22:17 ` Daniel Borkmann
2020-09-25 15:42 ` Daniel Borkmann
2020-09-25 15:52 ` Daniel Borkmann
2020-09-25 16:50 ` Andrii Nakryiko
2020-09-25 19:52 ` Daniel Borkmann [this message]
2020-09-25 6:13 ` Yonghong Song
2020-09-24 18:21 ` [PATCH bpf-next 5/6] bpf, selftests: use bpf_tail_call_static where appropriate Daniel Borkmann
2020-09-24 19:25 ` Maciej Fijalkowski
2020-09-24 22:03 ` Daniel Borkmann
2020-09-24 18:21 ` [PATCH bpf-next 6/6] bpf, selftests: add redirect_neigh selftest Daniel Borkmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bdddb94c-555c-3833-f82e-a53f83b1a77e@iogearbox.net \
--to=daniel@iogearbox.net \
--cc=andrii.nakryiko@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=john.fastabend@gmail.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).