Re: [PATCH] io_uring/io-wq: don't clear PF_IO_WORKER on exit

[Linus Torvalds]

On Tue, 13 Jun 2023 at 18:14, Jens Axboe <axboe@xxxxxxxxx> wrote:
>
> +       preempt_disable();
> +       current->worker_private = NULL;
> +       preempt_enable();

Yeah, that preempt_disable/enable cannot possibly make a difference in
any sane situation.

If you want to make clear that it should be one single write, do it
with WRITE_ONCE().

But realistically, that won't matter either. There's just no way a
sane compiler can make it do anything else, and just the plain

        current->worker_private = NULL;

will be equivalent.

If there are ordering concerns, then neither preemption nor
WRITE_ONCE() matter, but "smp_store_release()" would.

But then any readers should use "smp_load_acquire()" too.

However, in this case, I don't think any of that matters.

The actual backing store is free'd with kfree_rcu(), so any ordering
would be against the RCU grace period anyway. So the only ordering
that matters is, I think, that you set it to NULL *before* that
kfree_rcu() call, so that we know that "if somebody has seen a
non-NULL worker_private, then you still have a full RCU grace period
until it is gone".

Of course, that all still assumes that any read of worker_private
(from outside of 'current') is inside an RCU read-locked region. Which
isn't actually obviously true.

But at least for the case of io_wq_worker_running() and
io_wq_worker_sleeping, the call is always just for the current task.
So there are no ordering constraints at all. Not for preemption, not
for SMP, not for RCU. It's all entirely thread-local.

(That may not be obvious in the source code, since
io_wq_worker_sleeping/running gets a 'tsk' argument, but in the
context of the scheduler, 'tsk' is always just a cached copy of
'current').

End result: just do it as a plain store.  And I don't understand why
the free'ing of that data structure is RCU-delayed at all. There does
not seem to be any non-synchronous users of the worker_private pointer
at all. So I *think* that

        kfree_rcu(worker, rcu);

should just be

        kfree(worker);

and I wonder if that rcu-freeing was there to try to hide the bug.

But maybe I'm missing something.

            Linus