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.
>
> The choice of error name is odd, tbh.
I was trying to choose something that helpers will never pass back to the
program to avoid conflicts. Open to other suggestions (but it may not matter
depending on the discussion above).
