On Wed, Apr 12, 2023 at 09:43:06PM CEST, Alexei Starovoitov wrote:
> On Fri, Apr 07, 2023 at 04:57:48AM +0200, Kumar Kartikeya Dwivedi wrote:
> > On Fri, Apr 07, 2023 at 04:15:19AM CEST, Alexei Starovoitov wrote:
> > > On Fri, Apr 07, 2023 at 02:07:06AM +0200, Kumar Kartikeya Dwivedi wrote:
> > > > On Thu, Apr 06, 2023 at 04:21:39AM CEST, Alexei Starovoitov wrote:
> > > > > On Wed, Apr 05, 2023 at 02:42:34AM +0200, Kumar Kartikeya Dwivedi wrote:
> > > > > > @@ -759,6 +759,8 @@ BPF_CALL_4(bpf_loop, u32, nr_loops, void *, callback_fn, void *, callback_ctx,
> > > > > >
> > > > > > for (i = 0; i < nr_loops; i++) {
> > > > > > ret = callback((u64)i, (u64)(long)callback_ctx, 0, 0, 0);
> > > > > > + if (bpf_get_exception())
> > > > > > + return -EJUKEBOX;
> > > > >
> > > > > This is too slow.
> > > > > We cannot afford a call and conditional here.
> > > >
> > > > There are two more options here: have two variants, one with and without the
> > > > check (always_inline template and bpf_loop vs bpf_loop_except calling functions
> > > > which pass false/true) and dispatch to the appropriate one based on if callback
> > > > throws or not (so the cost is not paid for current users at all). Secondly, we
> > > > can avoid repeated calls by hoisting the call out and save the pointer to
> > > > exception state, then it's a bit less costly.
> > > >
> > > > > Some time ago folks tried bpf_loop() and went back to bounded loop, because
> > > > > the overhead of indirect call was not acceptable.
> > > > > After that we've added inlining of bpf_loop() to make overhead to the minimum.
> > > > > With prog->aux->exception[] approach it might be ok-ish,
> > > > > but my preference would be to disallow throw in callbacks.
> > > > > timer cb, rbtree_add cb are typically small.
> > > > > bpf_loop cb can be big, but we have open coded iterators now.
> > > > > So disabling asserts in cb-s is probably acceptable trade-off.
> > > >
> > > > If the only reason to avoid them is the added performance cost, we can work
> > > > towards eliminating that when bpf_throw is not used (see above). I agree that
> > > > supporting it everywhere means thinking about a lot more corner cases, but I
> > > > feel it would be less surprising if doing bpf_assert simply worked everywhere.
> > > > One of the other reasons is that if it's being used within a shared static
> > > > function that both main program and callbacks call into, it will be a bit
> > > > annoying that it doesn't work in one context.
> > >
> > > I hope with open coded iterators the only use case for callbacks will be timers,
> > > exception cb and rbtree_add. All three are special cases.
> >
> > There's also bpf_for_each_map_elem, bpf_find_vma, bpf_user_ringbuf_drain.
> >
> > > There is nothing to unwind in the timer case.
> > > Certinaly not allowed to rethrow in exception cb.
> > > less-like rbtree_add should be tiny. And it's gotta to be fast.
> >
> > I agree for some of the cases above they do not matter too much. The main one
> > was bpf_loop, and the ones I listed (like for_each_map) seem to be those where
> > people may do siginificant work in the callback.
> >
> > I think we can make it zero cost for programs that don't use it (even for the
> > kernel code), I was just hoping to be able to support it everywhere as a generic
> > helper to abort program execution without any special corner cases during usage.
> > The other main point was about code sharing of functions which makes use of
> > assertions. But I'm ok with dropping support for callbacks if you think it's not
> > worth it in the end.
>
> I think the unexpected run-time slowdown due to checks is a bigger problem.
> Since bpf_assert is a 'bad program detector' it should be as zero cost to run-time
> as possible. Hence I'm advocating for single load+jmp approach of prog->aux->exception.
I 100% agree, slow down is a big problem and a downside of the current version.
They need to be as lightweight as possible. It's too costly right now in this
set.
> The run-time matter more than ability to use assert in all conditions. I think
> we'd need two flavors of bpf_assert() asm macros: with and without
> bpf_throw(). They seem to be useful without actual throw. They will help the
> verifier understand the ranges of variables in both cases. The macros are the
> 99% of the feature. The actual throw mechanism is 1%. The unwinding and
> release of resource is a cost of using macros without explicit control flow in
> the bpf programs. The macros without throw might be good enough in many cases.
Ok, I'll update to allow for both variants. But just to confirm, do you want to
shelve automatic cleanup (part 2 for now), or want to add it? I'm not clear on
whether you agree or disagree it's necessary.
I'll try the prog->aux->exception route, but I must say that we're sort of
trading off simpler semantics/behavior for speedup in that case (which is
necessary, but it is a cost IMO). I'll post a version using that, but I'll also
add comparisons with the variant where we spill ptr to exception state and
load+jmp. It's an extra instruction but I will try to benchmark and see how much
difference it causes in practice (probably over the XDP benchmark using such
exceptions, since that's one of the most important performance-critical use
cases). If you agree, let's double down on whatever approach we choose after
analysing the difference?
