On Thu, 28 Aug 2025 at 19:00, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Wed, Aug 27, 2025 at 6:34 PM Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > On Wed, Aug 20, 2025 at 11:33 AM Andrii Nakryiko > > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > > > > > > I don't see why we can't consolidate internals of all these async > > > callbacks while maintaining a user-facing API that makes most sense > > > for each specific case. For task_work (and yes, I think for timers it > > > would make more sense as well), we are talking about a single > > > conceptual operation: just schedule a callback. So it makes sense to > > > have a single kfunc that expresses that. > > > > > > Having a split into init, set_callback, kick is unnecessary and > > > cumbersome. > > > > For timers the split api of init vs start_timer is mandatory, > > since it's common usage to start and stop the same timer before > > it fires. So the moving init operation (which allocates) and > > set_callback operation (that needs a pointer to callback) > > out of start/stop is a good thing, since the code that does start/stop > > may be far away and in a different file than init/set_callback. > > Ok, that makes sense, thanks for filling in the context. > > > That's how networking stack is written. > > Where I screwed things up is when I made bpf_timer_cancel() to also > > clear the callback to NULL. Not sure what I was thinking. > > The api wasn't modelled by existing kernel timer api, but > > by how the networking stack is using timers. > > Most of the time the started timer will be cancelled before firing. > > bpf_wq just followed bpf_timer api pattern. > > But, unlike timers, wq and task_work actually expect the callback > > to be called. It's rare to cancel wq/task_work. > > So for them the single 'just schedule' kfunc that allocates, > > sets callback, and schedules makes sense. > > For wq there is no bpf_wq_cancel. So there is a difference already. > > It's fine for bpf_timers, bpf_wq, bpf_task_work to have > > different set of kfuncs, since the usage pattern is different. > > > > agreed > > > Regarding state machine vs spinlock I think we should see > > through this approach with state machine. > > And if we convince ourselves that after all reviews it > > looks to be bug free, we should try to convert timer/wq > > to state machine as well to have a shared logic. > > yep, completely agree > > > Note irq_work from nmi is mandatory for timers too > > regardless of state machine or rqspinlock, > > since timers/wq take more locks inside, > > We cannot rqspinlock() + hrtimer_start() unconditionally. > > Ack. Once we unify all this, we can invest a bit more effort trying to > optimize away the need for irq_work when not under NMI (and maybe not > in irq). For timers this seems more important than for task_work, as > timers can be used significantly more frequently. Ok, let's go with this approach. I'll take a stab at addressing timers later on once Mykyta's set lands. > > But one step at a time, let's wait for Mykyta to come back from > vacation and update the patch set.
