On Mon, 2025-09-08 at 23:42 -0400, Andrii Nakryiko wrote:
> On Mon, Sep 8, 2025 at 1:39 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> >
> > On Mon, 2025-09-08 at 14:13 +0100, Mykyta Yatsenko wrote:
> >
> > [...]
> >
> > > > > +static void bpf_task_work_irq(struct irq_work *irq_work)
> > > > > +{
> > > > > + struct bpf_task_work_ctx *ctx = container_of(irq_work, struct bpf_task_work_ctx, irq_work);
> > > > > + enum bpf_task_work_state state;
> > > > > + int err;
> > > > > +
> > > > > + guard(rcu_tasks_trace)();
> > > > > +
> > > > > + if (cmpxchg(&ctx->state, BPF_TW_PENDING, BPF_TW_SCHEDULING) != BPF_TW_PENDING) {
> > > > > + bpf_task_work_ctx_put(ctx);
> > > > > + return;
> > > > > + }
> > > > Why are separate PENDING and SCHEDULING states needed?
> > > > Both indicate that the task had not been yet but is ready to be
> > > > submitted to task_work_add(). So, on a first glance it seems that
> > > > merging the two won't change the behaviour, what do I miss?
> >
> > > Yes, this is right, we may drop SCHEDULING state, it does not change any
> > > behavior compared to PENDING.
> > > The state check before task_work_add is needed anyway, so we won't
> > > remove much code here.
> > > I kept it just to be more consistent: with every state check we also
> > > transition state machine forward.
> >
> > Why is state check before task_work_add() mandatory?
> > You check for FREED in both branches of task_work_add(),
> > so there seem to be no issues with leaking ctx.
>
> Not really mandatory, but I think it is good to avoid even attempting
> to schedule task_work if the map element was already deleted?
> Technically, even that last FREED check in bpf_task_work_irq is not
> strictly necessary, we could have always let task_work callback
> execute and bail all the way there, but that seems too extreme (and
> task_work can be delayed by a lot for some special task states, I
> think).
>
> Also, keep in mind, this same state machine will be used for timers
> and wqs (at least we should try), and so, in general, being diligent
> about not doing doomed-to-be-failed-or-cancelled work is a good
> property.
Ack, thank you for explaining.
[...]
