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.
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.
