Thanks for taking a look, Stanislav! On Tue, 2022-07-12 at 11:07 -0700, sdf@xxxxxxxxxx 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@xxxxxxxxxxxxxx/
