Re: [PATCH v2] io_uring/lock: Keep spinlock release before wake_up()

[Jens Axboe]

On 9/23/26 3:13 PM, Gabriel Krisman Bertazi wrote:
> Xiaochuan Li <chuanx2070@xxxxxxx> writes:
> 
>> When CONFIG_PREEMPT_RT enable, raw_spin_lock() will preempt_disable()
> 
> The subject prefix is wrong.  It should be "io_uring/io-wq".
> 
> Also, It might be that I just didn't find it, but was there a v1 of this
> patch?  I can't find it on the list.

There should be a v1 and a v2, off the same thread.

>> which will trigger:
>> BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:46
>> in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 985654, name: iou-wrk-983605
>> preempt_count: 1, expected: 0
>> RCU nest depth: 0, expected: 0
>> CPU: 3 PID: 985654 Comm: iou-wrk-983605 Tainted: G        O  6.1.83-rt28-g19631eb82f21
>> stack:0        ppid:977479 flags:0x00000008
>> tgid:977553 prio:120 preempt:0x100000001 rcu_read_lock_nesting:0
>> used_cpu 3 wake_cpu 3 on_cpu 3 on_rq 1 migrate_dis 0
>> arrive:17423426040675 queued:0 prev_sum:33955300 sum_exec:33955300
>> Call trace:
>>  dump_backtrace.part.0+0xdc/0xec
>>  show_stack+0x1c/0x30
>>  dump_stack_lvl+0xac/0xc4
>>  dump_stack+0x14/0x30
>>  __might_resched+0x13c/0x170
>>  rt_spin_lock+0x34/0xc0
>>  __wake_up_common_lock+0x68/0xd0
>>  __wake_up+0x1c/0x24
>>  io_worker_handle_work+0x5b0/0x600
>>  io_wqe_worker+0xf4/0x310
>>  ret_from_fork+0x10/0x20
>>
>> Signed-off-by: Xiaochuan Li <chuanx2070@xxxxxxx>
>> ---
>> Changes in v2:
>> io_uring/io-wq: fix lockdep warning by deferring hash wake up outside acct->lock
>>
>> The stall wake up path in io_get_next_work() holds acct->lock while
>> calling wake_up() on the hash wait queue, which creates lock ordering
>> acct->lock -> hash->wait.lock and triggers lockdep circular dependency
>> warning.
>>
>> The previous approach of temporarily dropping and retaking acct->lock
>> is racy and juggles the lock unnecessarily. Instead, add a need_wake
>> output flag to io_get_next_work() and defer the wake_up() to the outer
>> worker loop, after acct->lock has been released.
> 
> what previous approach?
> 
>>
>> This preserves the calling convention that io_get_next_work() returns
>> with acct->lock held, removes the lock inversion, and avoids any racy
>> sleeper checks outside of the lock. Drop the wq_has_sleeper check as
>> bare wake_up is safe and the optimization is not worth the complexity.
> 
> Either way, these paragraphs should be part of the commit message. By
> putting them after the ---, they get dropped at commit-time.

I rewrote most of the commit message when applying, fwiw. You can find
it in my tree.

-- 
Jens Axboe