public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()
@ 2021-08-19 15:48 Jens Axboe
  2021-08-19 17:26 ` Hao Xu
  0 siblings, 1 reply; 7+ messages in thread
From: Jens Axboe @ 2021-08-19 15:48 UTC (permalink / raw)
  To: io-uring

We have two checks of task->flags & PF_EXITING left:

1) In io_req_task_submit(), which is called in task_work and hence always
   in the context of the original task. That means that
   req->task == current, and hence checking ->flags is totally fine.

2) In io_poll_rewait(), where we need to stop re-arming poll to prevent
   it interfering with cancelation. Here, req->task is not necessarily
   current, and hence the check is racy. Use the ctx refs state instead
   to check if we need to cancel this request or not.

Signed-off-by: Jens Axboe <[email protected]>

---

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 30edc329d803..ffce959c2370 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2114,6 +2114,7 @@ static void io_req_task_submit(struct io_kiocb *req)
 
 	/* ctx stays valid until unlock, even if we drop all ours ctx->refs */
 	mutex_lock(&ctx->uring_lock);
+	/* req->task == current here, checking PF_EXITING is safe */
 	if (likely(!(req->task->flags & PF_EXITING)))
 		__io_queue_sqe(req);
 	else
@@ -4895,7 +4896,11 @@ static bool io_poll_rewait(struct io_kiocb *req, struct io_poll_iocb *poll)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 
-	if (unlikely(req->task->flags & PF_EXITING))
+	/*
+	 * Pairs with spin_unlock() in percpu_ref_kill()
+	 */
+	smp_rmb();
+	if (unlikely(percpu_ref_is_dying(&ctx->refs)))
 		WRITE_ONCE(poll->canceled, true);
 
 	if (!req->result && !READ_ONCE(poll->canceled)) {

-- 
Jens Axboe


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()
  2021-08-19 15:48 [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait() Jens Axboe
@ 2021-08-19 17:26 ` Hao Xu
  2021-08-19 17:29   ` Jens Axboe
  0 siblings, 1 reply; 7+ messages in thread
From: Hao Xu @ 2021-08-19 17:26 UTC (permalink / raw)
  To: Jens Axboe, io-uring

在 2021/8/19 下午11:48, Jens Axboe 写道:
> We have two checks of task->flags & PF_EXITING left:
> 
> 1) In io_req_task_submit(), which is called in task_work and hence always
>     in the context of the original task. That means that
>     req->task == current, and hence checking ->flags is totally fine.
> 
> 2) In io_poll_rewait(), where we need to stop re-arming poll to prevent
>     it interfering with cancelation. Here, req->task is not necessarily
>     current, and hence the check is racy. Use the ctx refs state instead
>     to check if we need to cancel this request or not.
Hi Jens,
I saw cases that io_req_task_submit() and io_poll_rewait() in one
function, why one is safe and the other one not? btw, it seems both two
executes in task_work context..and task_work_add() may fail and then
work goes to system_wq, is that case safe?
> 
> Signed-off-by: Jens Axboe <[email protected]>
> 
> ---
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 30edc329d803..ffce959c2370 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -2114,6 +2114,7 @@ static void io_req_task_submit(struct io_kiocb *req)
>   
>   	/* ctx stays valid until unlock, even if we drop all ours ctx->refs */
>   	mutex_lock(&ctx->uring_lock);
> +	/* req->task == current here, checking PF_EXITING is safe */
>   	if (likely(!(req->task->flags & PF_EXITING)))
>   		__io_queue_sqe(req);
>   	else
> @@ -4895,7 +4896,11 @@ static bool io_poll_rewait(struct io_kiocb *req, struct io_poll_iocb *poll)
>   {
>   	struct io_ring_ctx *ctx = req->ctx;
>   
> -	if (unlikely(req->task->flags & PF_EXITING))
> +	/*
> +	 * Pairs with spin_unlock() in percpu_ref_kill()
> +	 */
> +	smp_rmb();
> +	if (unlikely(percpu_ref_is_dying(&ctx->refs)))
>   		WRITE_ONCE(poll->canceled, true);
>   
>   	if (!req->result && !READ_ONCE(poll->canceled)) {
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()
  2021-08-19 17:26 ` Hao Xu
@ 2021-08-19 17:29   ` Jens Axboe
  2021-08-19 17:36     ` Jens Axboe
  2021-08-19 17:36     ` Hao Xu
  0 siblings, 2 replies; 7+ messages in thread
From: Jens Axboe @ 2021-08-19 17:29 UTC (permalink / raw)
  To: Hao Xu, io-uring

On 8/19/21 11:26 AM, Hao Xu wrote:
> 在 2021/8/19 下午11:48, Jens Axboe 写道:
>> We have two checks of task->flags & PF_EXITING left:
>>
>> 1) In io_req_task_submit(), which is called in task_work and hence always
>>     in the context of the original task. That means that
>>     req->task == current, and hence checking ->flags is totally fine.
>>
>> 2) In io_poll_rewait(), where we need to stop re-arming poll to prevent
>>     it interfering with cancelation. Here, req->task is not necessarily
>>     current, and hence the check is racy. Use the ctx refs state instead
>>     to check if we need to cancel this request or not.
> Hi Jens,
> I saw cases that io_req_task_submit() and io_poll_rewait() in one
> function, why one is safe and the other one not? btw, it seems both two
> executes in task_work context..and task_work_add() may fail and then
> work goes to system_wq, is that case safe?

io_req_task_submit() is guaranteed to be run in the task that is req->task,
io_poll_rewait() is not. The latter can get called from eg the poll
waitqueue handling, which is not run from the task in question.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()
  2021-08-19 17:29   ` Jens Axboe
@ 2021-08-19 17:36     ` Jens Axboe
  2021-08-19 17:39       ` Hao Xu
  2021-08-19 17:36     ` Hao Xu
  1 sibling, 1 reply; 7+ messages in thread
From: Jens Axboe @ 2021-08-19 17:36 UTC (permalink / raw)
  To: Hao Xu, io-uring

On 8/19/21 11:29 AM, Jens Axboe wrote:
> On 8/19/21 11:26 AM, Hao Xu wrote:
>> 在 2021/8/19 下午11:48, Jens Axboe 写道:
>>> We have two checks of task->flags & PF_EXITING left:
>>>
>>> 1) In io_req_task_submit(), which is called in task_work and hence always
>>>     in the context of the original task. That means that
>>>     req->task == current, and hence checking ->flags is totally fine.
>>>
>>> 2) In io_poll_rewait(), where we need to stop re-arming poll to prevent
>>>     it interfering with cancelation. Here, req->task is not necessarily
>>>     current, and hence the check is racy. Use the ctx refs state instead
>>>     to check if we need to cancel this request or not.
>> Hi Jens,
>> I saw cases that io_req_task_submit() and io_poll_rewait() in one
>> function, why one is safe and the other one not? btw, it seems both two
>> executes in task_work context..and task_work_add() may fail and then
>> work goes to system_wq, is that case safe?
> 
> io_req_task_submit() is guaranteed to be run in the task that is req->task,
> io_poll_rewait() is not. The latter can get called from eg the poll
> waitqueue handling, which is not run from the task in question.

Pavel nudged me, and in the 5.15 branch we actually only do run rewait
from the task itself. So this patch isn't needed, we can ignore it!
Might just augment it with a comment, like it was done for submit.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()
  2021-08-19 17:29   ` Jens Axboe
  2021-08-19 17:36     ` Jens Axboe
@ 2021-08-19 17:36     ` Hao Xu
  2021-08-19 17:37       ` Jens Axboe
  1 sibling, 1 reply; 7+ messages in thread
From: Hao Xu @ 2021-08-19 17:36 UTC (permalink / raw)
  To: Jens Axboe, io-uring

在 2021/8/20 上午1:29, Jens Axboe 写道:
> On 8/19/21 11:26 AM, Hao Xu wrote:
>> 在 2021/8/19 下午11:48, Jens Axboe 写道:
>>> We have two checks of task->flags & PF_EXITING left:
>>>
>>> 1) In io_req_task_submit(), which is called in task_work and hence always
>>>      in the context of the original task. That means that
>>>      req->task == current, and hence checking ->flags is totally fine.
>>>
>>> 2) In io_poll_rewait(), where we need to stop re-arming poll to prevent
>>>      it interfering with cancelation. Here, req->task is not necessarily
>>>      current, and hence the check is racy. Use the ctx refs state instead
>>>      to check if we need to cancel this request or not.
>> Hi Jens,
>> I saw cases that io_req_task_submit() and io_poll_rewait() in one
>> function, why one is safe and the other one not? btw, it seems both two
>> executes in task_work context..and task_work_add() may fail and then
>> work goes to system_wq, is that case safe?
I've got answer for the second question..
> 
> io_req_task_submit() is guaranteed to be run in the task that is req->task,
> io_poll_rewait() is not. The latter can get called from eg the poll
> waitqueue handling, which is not run from the task in question.
I only found io_poll_rewait() call in io_async_task_func() and
io_poll_task_func(), both are in task_work
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()
  2021-08-19 17:36     ` Hao Xu
@ 2021-08-19 17:37       ` Jens Axboe
  0 siblings, 0 replies; 7+ messages in thread
From: Jens Axboe @ 2021-08-19 17:37 UTC (permalink / raw)
  To: Hao Xu, io-uring

On 8/19/21 11:36 AM, Hao Xu wrote:
> 在 2021/8/20 上午1:29, Jens Axboe 写道:
>> On 8/19/21 11:26 AM, Hao Xu wrote:
>>> 在 2021/8/19 下午11:48, Jens Axboe 写道:
>>>> We have two checks of task->flags & PF_EXITING left:
>>>>
>>>> 1) In io_req_task_submit(), which is called in task_work and hence always
>>>>      in the context of the original task. That means that
>>>>      req->task == current, and hence checking ->flags is totally fine.
>>>>
>>>> 2) In io_poll_rewait(), where we need to stop re-arming poll to prevent
>>>>      it interfering with cancelation. Here, req->task is not necessarily
>>>>      current, and hence the check is racy. Use the ctx refs state instead
>>>>      to check if we need to cancel this request or not.
>>> Hi Jens,
>>> I saw cases that io_req_task_submit() and io_poll_rewait() in one
>>> function, why one is safe and the other one not? btw, it seems both two
>>> executes in task_work context..and task_work_add() may fail and then
>>> work goes to system_wq, is that case safe?
> I've got answer for the second question..
>>
>> io_req_task_submit() is guaranteed to be run in the task that is req->task,
>> io_poll_rewait() is not. The latter can get called from eg the poll
>> waitqueue handling, which is not run from the task in question.
> I only found io_poll_rewait() call in io_async_task_func() and
> io_poll_task_func(), both are in task_work

Yeah see followup, my information was outdated, we only do rewait from the
right context at this point. Hence the PF_EXITING check is actually fine.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait()
  2021-08-19 17:36     ` Jens Axboe
@ 2021-08-19 17:39       ` Hao Xu
  0 siblings, 0 replies; 7+ messages in thread
From: Hao Xu @ 2021-08-19 17:39 UTC (permalink / raw)
  To: Jens Axboe, io-uring

在 2021/8/20 上午1:36, Jens Axboe 写道:
> On 8/19/21 11:29 AM, Jens Axboe wrote:
>> On 8/19/21 11:26 AM, Hao Xu wrote:
>>> 在 2021/8/19 下午11:48, Jens Axboe 写道:
>>>> We have two checks of task->flags & PF_EXITING left:
>>>>
>>>> 1) In io_req_task_submit(), which is called in task_work and hence always
>>>>      in the context of the original task. That means that
>>>>      req->task == current, and hence checking ->flags is totally fine.
>>>>
>>>> 2) In io_poll_rewait(), where we need to stop re-arming poll to prevent
>>>>      it interfering with cancelation. Here, req->task is not necessarily
>>>>      current, and hence the check is racy. Use the ctx refs state instead
>>>>      to check if we need to cancel this request or not.
>>> Hi Jens,
>>> I saw cases that io_req_task_submit() and io_poll_rewait() in one
>>> function, why one is safe and the other one not? btw, it seems both two
>>> executes in task_work context..and task_work_add() may fail and then
>>> work goes to system_wq, is that case safe?
>>
>> io_req_task_submit() is guaranteed to be run in the task that is req->task,
>> io_poll_rewait() is not. The latter can get called from eg the poll
>> waitqueue handling, which is not run from the task in question.
> 
> Pavel nudged me, and in the 5.15 branch we actually only do run rewait
> from the task itself. So this patch isn't needed, we can ignore it!
> Might just augment it with a comment, like it was done for submit.
saw this after my second email. cool!
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-08-19 17:39 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-19 15:48 [PATCH] io_uring: remove PF_EXITING checking in io_poll_rewait() Jens Axboe
2021-08-19 17:26 ` Hao Xu
2021-08-19 17:29   ` Jens Axboe
2021-08-19 17:36     ` Jens Axboe
2021-08-19 17:39       ` Hao Xu
2021-08-19 17:36     ` Hao Xu
2021-08-19 17:37       ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox