Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <2392dcb4-71f4-1109-614b-4e2083c0941e@kernel.dk>
 <20230614005449.awc2ncxl5lb2eg6m@zlang-mailbox> <5d5ccbb1-784c-52b3-3748-2cf7b5cf01ef@kernel.dk>
In-Reply-To: <5d5ccbb1-784c-52b3-3748-2cf7b5cf01ef@kernel.dk>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed, 14 Jun 2023 10:44:53 -0700
Message-ID: <CAHk-=wiotpcKvBWGneGjNA4eOGUsY+KTMCVsMxsGhXGCg=n=bA@mail.gmail.com>
Subject: Re: [PATCH] io_uring/io-wq: don't clear PF_IO_WORKER on exit
To:     Jens Axboe <axboe@kernel.dk>
Cc:     Zorro Lang <zlang@redhat.com>, io-uring <io-uring@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Dave Chinner <david@fromorbit.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 13 Jun 2023 at 18:14, Jens Axboe <axboe@kernel.dk> 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