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