[PATCH 5.13] io_uring: don't set IORING_SQ_NEED_WAKEUP when sqthread is dying

[PATCH 5.13] io_uring: don't set IORING_SQ_NEED_WAKEUP when sqthread is dying

[Hao Xu]

we don't need to re-fork the sqthread over exec, so no need to set
IORING_SQ_NEED_WAKEUP when sqthread is dying.

Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 fs/io_uring.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 6b578c380e73..92dcd1c21516 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6897,8 +6897,6 @@ static int io_sq_thread(void *data)
 
 	io_uring_cancel_sqpoll(sqd);
 	sqd->thread = NULL;
-	list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
-		io_ring_set_wakeup_flag(ctx);
 	io_run_task_work();
 	io_run_task_work_head(&sqd->park_task_work);
 	mutex_unlock(&sqd->lock);
-- 
1.8.3.1

Re: [PATCH 5.13] io_uring: don't set IORING_SQ_NEED_WAKEUP when sqthread is dying

[Pavel Begunkov]

On 4/27/21 1:45 PM, Hao Xu wrote:
> we don't need to re-fork the sqthread over exec, so no need to set
> IORING_SQ_NEED_WAKEUP when sqthread is dying.

It forces users to call io_uring_enter() for it to return
-EOWNERDEAD. Consider that scenario with the ring given
away to some other task not in current group, e.g. via socket.

if (ctx->flags & IORING_SETUP_SQPOLL) {
	io_cqring_overflow_flush(ctx, false);

	ret = -EOWNERDEAD;
	if (unlikely(ctx->sq_data->thread == NULL)) {
		goto out;
	}
	...
}

btw, can use a comment

> 
> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
> ---
>  fs/io_uring.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 6b578c380e73..92dcd1c21516 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -6897,8 +6897,6 @@ static int io_sq_thread(void *data)
>  
>  	io_uring_cancel_sqpoll(sqd);
>  	sqd->thread = NULL;
> -	list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
> -		io_ring_set_wakeup_flag(ctx);
>  	io_run_task_work();
>  	io_run_task_work_head(&sqd->park_task_work);
>  	mutex_unlock(&sqd->lock);
> 

-- 
Pavel Begunkov

Re: [PATCH 5.13] io_uring: don't set IORING_SQ_NEED_WAKEUP when sqthread is dying

[Hao Xu]

在 2021/4/27 下午9:13, Pavel Begunkov 写道:
> On 4/27/21 1:45 PM, Hao Xu wrote:
>> we don't need to re-fork the sqthread over exec, so no need to set
>> IORING_SQ_NEED_WAKEUP when sqthread is dying.
> 
> It forces users to call io_uring_enter() for it to return
> -EOWNERDEAD. Consider that scenario with the ring given
> away to some other task not in current group, e.g. via socket.
> 
Ah, I see. Thank you Pavel.
> if (ctx->flags & IORING_SETUP_SQPOLL) {
> 	io_cqring_overflow_flush(ctx, false);
> 
> 	ret = -EOWNERDEAD;
> 	if (unlikely(ctx->sq_data->thread == NULL)) {
> 		goto out;
> 	}
> 	...
> }
> 
> btw, can use a comment
> 
>>
>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
>> ---
>>   fs/io_uring.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 6b578c380e73..92dcd1c21516 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -6897,8 +6897,6 @@ static int io_sq_thread(void *data)
>>   
>>   	io_uring_cancel_sqpoll(sqd);
>>   	sqd->thread = NULL;
>> -	list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
>> -		io_ring_set_wakeup_flag(ctx);
>>   	io_run_task_work();
>>   	io_run_task_work_head(&sqd->park_task_work);
>>   	mutex_unlock(&sqd->lock);
>>
> 

Re: [PATCH 5.13] io_uring: don't set IORING_SQ_NEED_WAKEUP when sqthread is dying

[Hao Xu]

在 2021/4/27 下午10:35, Hao Xu 写道:
> 在 2021/4/27 下午9:13, Pavel Begunkov 写道:
>> On 4/27/21 1:45 PM, Hao Xu wrote:
>>> we don't need to re-fork the sqthread over exec, so no need to set
>>> IORING_SQ_NEED_WAKEUP when sqthread is dying.
>>
>> It forces users to call io_uring_enter() for it to return
>> -EOWNERDEAD. Consider that scenario with the ring given
>> away to some other task not in current group, e.g. via socket.
>>
> Ah, I see. Thank you Pavel.
Here I've a question: for processes that aren't in same group, io_uring
is now designed that sqthread cannot be shared between these processes?
But It seems if users do fork(), they can still call io_uring_enter()
in the forked task?
>> if (ctx->flags & IORING_SETUP_SQPOLL) {
>>     io_cqring_overflow_flush(ctx, false);
>>
>>     ret = -EOWNERDEAD;
>>     if (unlikely(ctx->sq_data->thread == NULL)) {
>>         goto out;
>>     }
>>     ...
>> }
>>
>> btw, can use a comment
>>
>>>
>>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
>>> ---
>>>   fs/io_uring.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>> index 6b578c380e73..92dcd1c21516 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -6897,8 +6897,6 @@ static int io_sq_thread(void *data)
>>>       io_uring_cancel_sqpoll(sqd);
>>>       sqd->thread = NULL;
>>> -    list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
>>> -        io_ring_set_wakeup_flag(ctx);
>>>       io_run_task_work();
>>>       io_run_task_work_head(&sqd->park_task_work);
>>>       mutex_unlock(&sqd->lock);
>>>
>>

Re: [PATCH 5.13] io_uring: don't set IORING_SQ_NEED_WAKEUP when sqthread is dying

[Pavel Begunkov]

On 4/28/21 7:38 AM, Hao Xu wrote:
> 在 2021/4/27 下午10:35, Hao Xu 写道:
>> 在 2021/4/27 下午9:13, Pavel Begunkov 写道:
>>> On 4/27/21 1:45 PM, Hao Xu wrote:
>>>> we don't need to re-fork the sqthread over exec, so no need to set
>>>> IORING_SQ_NEED_WAKEUP when sqthread is dying.
>>>
>>> It forces users to call io_uring_enter() for it to return
>>> -EOWNERDEAD. Consider that scenario with the ring given
>>> away to some other task not in current group, e.g. via socket.
>>>
>> Ah, I see. Thank you Pavel.
> Here I've a question: for processes that aren't in same group, io_uring
> is now designed that sqthread cannot be shared between these processes?

Right, sqthread can't be shared by rings created in different thread
groups, but it doesn't mean the ring itself can't be shared.

> But It seems if users do fork(), they can still call io_uring_enter()
> in the forked task?

IIRC, forking CLONE_THREAD task is discouraged but allowed, but in any
case it's ok from the io_uring perspective. Requests will be fully
executed in the context of the sqpoll task

>>> if (ctx->flags & IORING_SETUP_SQPOLL) {
>>>     io_cqring_overflow_flush(ctx, false);
>>>
>>>     ret = -EOWNERDEAD;
>>>     if (unlikely(ctx->sq_data->thread == NULL)) {
>>>         goto out;
>>>     }
>>>     ...
>>> }
>>>
>>> btw, can use a comment
>>>
>>>>
>>>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
>>>> ---
>>>>   fs/io_uring.c | 2 --
>>>>   1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index 6b578c380e73..92dcd1c21516 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -6897,8 +6897,6 @@ static int io_sq_thread(void *data)
>>>>       io_uring_cancel_sqpoll(sqd);
>>>>       sqd->thread = NULL;
>>>> -    list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
>>>> -        io_ring_set_wakeup_flag(ctx);
>>>>       io_run_task_work();
>>>>       io_run_task_work_head(&sqd->park_task_work);
>>>>       mutex_unlock(&sqd->lock);
>>>>
>>>
> 

-- 
Pavel Begunkov