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

[sdf@google.com]

On 07/12, Delyan Kratunov wrote:
> 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/


All valid points. I'm assuming Alexei will take a closer look at this
eventually since I don't have a ton of context about timers :-(