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.
