Re: [PATCH] io_uring: consistently use rcu semantics with sqpoll thread

[Jens Axboe <axboe@kernel.dk>]

On 6/10/25 2:45 PM, Jens Axboe wrote:
> On 6/10/25 2:20 PM, Keith Busch wrote:
>> On Tue, Jun 10, 2025 at 02:04:41PM -0600, Jens Axboe wrote:
>>> On 6/10/25 1:30 PM, Keith Busch wrote:
>>>> From: Keith Busch <kbusch@kernel.org>
>>>>
>>>> It is already dereferenced with rcu read protection, so it needs to be
>>>> annotated as such, and consistently use rcu helpers for access and
>>>> assignment.
>>>
>>> There are some bits in io_uring.c that access it, which probably need
>>> some attention too I think. One of them a bit trickier.
>>
>> Oh, sure is. I just ran 'make C=1' on the originally affected files, but
>> should have ran it on all of io_uring/.
>>
>> I think the below should clear up the new warnings. I think it's safe to
>> hold the rcu read lock for the tricky one as io_wq_cancel_cb() doesn't
>> appear to make any blocking calls.
> 
> It _probably_ is, but that's entirely untested. Right now it looks fine,
> for a variety of reasons like submitting work that's marked cancel
> should not be doing anything with it really. But it doesn't feel me with
> joy, particularly as only the somewhat uncommon SQPOLL is the one that
> will do it.
> 
> The io_sq_thread_park() parts in the patch also look broken, as an
> rcu_access_pointer() is being passed into wake_up_process(). It should
> all be fine, but it's now a case of instrumentation actively making the
> code more confusing to read.
> 
> I think we might be better off leaving the sparse warnings and doing a
> proper io_sq_data accessor thing for this, rather than try and paper
> over the sparse warnings.

Ah we can probably get by with just a bit more work, something like
this per section should do it.

diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c
index 0625a421626f..8852e104ed68 100644
--- a/io_uring/sqpoll.c
+++ b/io_uring/sqpoll.c
@@ -46,13 +46,18 @@ void io_sq_thread_unpark(struct io_sq_data *sqd)
 void io_sq_thread_park(struct io_sq_data *sqd)
 	__acquires(&sqd->lock)
 {
-	WARN_ON_ONCE(data_race(sqd->thread) == current);
+	struct task_struct *tsk;
 
 	atomic_inc(&sqd->park_pending);
 	set_bit(IO_SQ_THREAD_SHOULD_PARK, &sqd->state);
 	mutex_lock(&sqd->lock);
-	if (sqd->thread)
-		wake_up_process(sqd->thread);
+	rcu_read_lock();
+	tsk = rcu_dereference(sqd->thread);
+	if (tsk) {
+		WARN_ON_ONCE(tsk == current);
+		wake_up_process(tsk);
+	}
+	rcu_read_unlock();
 }
 
 void io_sq_thread_stop(struct io_sq_data *sqd)

-- 
Jens Axboe