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