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

[Gabriel Krisman Bertazi]

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.

> 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.

-- 
Gabriel Krisman Bertazi