| Message ID | 20230524052801.369798-1-wenwen.chen@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: unlock sqd->lock before sq thread release CPU | expand |
On 5/23/23 11:28?PM, Wenwen Chen wrote: > The sq thread actively releases CPU resources by calling the > cond_resched() and schedule() interfaces when it is idle. Therefore, > more resources are available for other threads to run. > > There exists a problem in sq thread: it does not unlock sqd->lock before > releasing CPU resources every time. This makes other threads pending on > sqd->lock for a long time. For example, the following interfaces all > require sqd->lock: io_sq_offload_create(), io_register_iowq_max_workers() > and io_ring_exit_work(). > > Before the sq thread releases CPU resources, unlocking sqd->lock will > provide the user a better experience because it can respond quickly to > user requests. > > Signed-off-by: Kanchan Joshi<joshi.k@samsung.com> > Signed-off-by: Wenwen Chen<wenwen.chen@samsung.com> > --- > io_uring/sqpoll.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c > index 9db4bc1f521a..759c80fb4afa 100644 > --- a/io_uring/sqpoll.c > +++ b/io_uring/sqpoll.c > @@ -255,7 +255,9 @@ static int io_sq_thread(void *data) > sqt_spin = true; > > if (sqt_spin || !time_after(jiffies, timeout)) { > + mutex_unlock(&sqd->lock); > cond_resched(); > + mutex_lock(&sqd->lock); > if (sqt_spin) > timeout = jiffies + sqd->sq_thread_idle; > continue; Since this is the spin case, and we expect (by far) most of these to NOT need a reschedule, I think we should do: if (need_resched()) { mutex_unlock(&sqd->lock); cond_resched(); mutex_lock(&sqd->lock); } to make that lock shuffle dependent on the need to reschedule. And since we're marking the timeout at that point, timeout should be assigned first as far as I can tell. So in total: if (sqt_spin || !time_after(jiffies, timeout)) { if (sqt_spin) timeout = jiffies + sqd->sq_thread_idle; if (unlikely(need_resched())) { mutex_unlock(&sqd->lock); cond_resched(); mutex_lock(&sqd->lock); } continue; } would probably be the better fix.
diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c index 9db4bc1f521a..759c80fb4afa 100644 --- a/io_uring/sqpoll.c +++ b/io_uring/sqpoll.c @@ -255,7 +255,9 @@ static int io_sq_thread(void *data) sqt_spin = true; if (sqt_spin || !time_after(jiffies, timeout)) { + mutex_unlock(&sqd->lock); cond_resched(); + mutex_lock(&sqd->lock); if (sqt_spin) timeout = jiffies + sqd->sq_thread_idle; continue;
The sq thread actively releases CPU resources by calling the cond_resched() and schedule() interfaces when it is idle. Therefore, more resources are available for other threads to run. There exists a problem in sq thread: it does not unlock sqd->lock before releasing CPU resources every time. This makes other threads pending on sqd->lock for a long time. For example, the following interfaces all require sqd->lock: io_sq_offload_create(), io_register_iowq_max_workers() and io_ring_exit_work(). Before the sq thread releases CPU resources, unlocking sqd->lock will provide the user a better experience because it can respond quickly to user requests. Signed-off-by: Kanchan Joshi<joshi.k@samsung.com> Signed-off-by: Wenwen Chen<wenwen.chen@samsung.com> --- io_uring/sqpoll.c | 2 ++ 1 file changed, 2 insertions(+)