Re: [PATCH RFC bpf-next 0/3] Execution context callbacks

[Delyan Kratunov <delyank@fb.com>]

Thanks for taking a look, Stanislav!

On Tue, 2022-07-12 at 11:07 -0700, sdf@google.com wrote:
> *snip*
> > 2. The callback arguments need to be in a map. We can currently express  
> > helper arguments taking a
> > pointer to a map value but not a pointer to _within_ a map value. Should  
> > we add a new argument
> > type or should we just pass the map value pointer to the callback?
> 
> Passing map value pointer (as you do in the selftest) seems fine; do
> you think we need more flexibility here?

I think it makes a cleaner and more familiar API - the pointer to my data that I give
to the submission function is the one I get in the callback. Requiring it to be a map
value is a little bit quirky (it's not really my data it's pointing to!). I don't
know if it's a lot of work in the verifier to iron out this quirk but if it's
reasonable, I'd be happy to make the developer experience a little more predictable.

> > 3. A lot of the map handling code is verbatim from bpf_timer. This feels  
> > icky but I'm not sure if it
> > justifies a refactor quite yet. Opinions welcome.
> 
> +1, it does seem very close to a timer with expiry time == 0.
> 
> I don't know what's the exact usecase you're trying to solve exactly,

The primary motivating examples are 1) GFP_ATOMIC usage is not safe in NMI aiui, so
switching allocations to hardirq helps and 2) copy_from_user in tracing programs (nmi
or softirq when using software clocks). The latter shows up in insidious ways like
build id not being reliable when retrieving stack traces ([1] is a thread from a
while ago about it).

> but have you though of maybe initially supporting something like:
> 
> bpf_timer_init(&timer, map, SOME_NEW_DEFERRED_NMI_ONLY_FLAG);
> bpf_timer_set_callback(&timer, cg);
> bpf_timer_start(&timer, 0, 0);
> 
> If you init a timer with that special flag, I'm assuming you can have
> special cases in the existing helpers to simulate the delayed work?

Potentially but I have some reservations about drawing this equivalence.

> Then, the verifier changes should be minimal it seems.
> 
> OTOH, having a separate set of helpers seems more clear API-wise :-/

The primary way this differs from timers is that timers already specify an execution
context - the callback will be called from a softirq. 

It doesn't make sense to me to have some "timers" (but only 0-delay, super-special
timers) run in hardirq or, more confusingly, user context. At that point, there's
little in the API to express these differences, (e.g., bpf_copy_from_user_task is
accessible in *this* callback) and the verifier work will be far more challenging (if
at all possible since the init and the set_callback would be split).

I think it's worth thinking about how to unify the handling of timer-like map value
members but I don't think it's worth it trying to shoehorn this functionality into
existing infra.

> *snip*

  [1]: https://lore.kernel.org/bpf/CA+khW7gh=vO8m-_SVnwWwj7kv+EDeUPcuWFqebf2Zmi9T_oEAQ@mail.gmail.com/