[PATCH] io_uring:IORING_SETUP_SQPOLL don't need to enter io_cqring_wait

[PATCH] io_uring:IORING_SETUP_SQPOLL don't need to enter io_cqring_wait

[wu860403]

From: Liming Wu <19092205@suning.com>

When SETUP_IOPOLL and SETUP_SQPOLL are both enabled, app don't
need to enter io_cqring_wait too. If I misunderstand, please give
me some advise.

Signed-off-by Liming Wu <19092205@suning.com>
---
 io_uring.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/io_uring.c b/io_uring.c
index b12d33b..36e884f 100644
--- a/io_uring.c
+++ b/io_uring.c
@@ -7418,11 +7418,12 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
 		 * polling again, they can rely on io_sq_thread to do polling
 		 * work, which can reduce cpu usage and uring_lock contention.
 		 */
-		if (ctx->flags & IORING_SETUP_IOPOLL &&
-		    !(ctx->flags & IORING_SETUP_SQPOLL)) {
-			ret = io_iopoll_check(ctx, &nr_events, min_complete);
-		} else {
-			ret = io_cqring_wait(ctx, min_complete, sig, sigsz);
+		if (!(ctx->flags & IORING_SETUP_SQPOLL)) {
+		    if (ctx->flags & IORING_SETUP_IOPOLL) {
+		    	ret = io_iopoll_check(ctx, &nr_events, min_complete);
+		    } else {
+		    	ret = io_cqring_wait(ctx, min_complete, sig, sigsz);
+		    }
 		}
 	}
 
-- 
1.8.3.1

Re: [PATCH] io_uring:IORING_SETUP_SQPOLL don't need to enter io_cqring_wait

[Jens Axboe]

On 4/7/20 1:44 AM, wu860403@gmail.com wrote:
> From: Liming Wu <19092205@suning.com>
> 
> When SETUP_IOPOLL and SETUP_SQPOLL are both enabled, app don't
> need to enter io_cqring_wait too. If I misunderstand, please give
> me some advise.

The logic should be as follows:

flags			method
---------------------------------------------
0			io_cqring_wait()
IOPOLL			io_iopoll_check()
IOPOLL | SQPOLL		io_cqring_wait()
SQPOLL			io_cqring_wait()

The reasoning being that we do want to enter cqring_wait() for SQPOLL,
as the application may want to wait for completions. Even with IOPOLL
set. As far as I can tell, the current code is correct, as long as we
know SQPOLL will always poll for events for us.

So I'm curious why you think your patch is needed? Leaving it below and
CC'ing Xiaoguang, who made the most recent change, so he can comment.

> Signed-off-by Liming Wu <19092205@suning.com>
> ---
>  io_uring.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/io_uring.c b/io_uring.c
> index b12d33b..36e884f 100644
> --- a/io_uring.c
> +++ b/io_uring.c
> @@ -7418,11 +7418,12 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
>  		 * polling again, they can rely on io_sq_thread to do polling
>  		 * work, which can reduce cpu usage and uring_lock contention.
>  		 */
> -		if (ctx->flags & IORING_SETUP_IOPOLL &&
> -		    !(ctx->flags & IORING_SETUP_SQPOLL)) {
> -			ret = io_iopoll_check(ctx, &nr_events, min_complete);
> -		} else {
> -			ret = io_cqring_wait(ctx, min_complete, sig, sigsz);
> +		if (!(ctx->flags & IORING_SETUP_SQPOLL)) {
> +		    if (ctx->flags & IORING_SETUP_IOPOLL) {
> +		    	ret = io_iopoll_check(ctx, &nr_events, min_complete);
> +		    } else {
> +		    	ret = io_cqring_wait(ctx, min_complete, sig, sigsz);
> +		    }
>  		}
>  	}
>  
> 


-- 
Jens Axboe

Re: [PATCH] io_uring:IORING_SETUP_SQPOLL don't need to enter io_cqring_wait

[Xiaoguang Wang]

hi,

> On 4/7/20 1:44 AM, wu860403@gmail.com wrote:
>> From: Liming Wu <19092205@suning.com>
>>
>> When SETUP_IOPOLL and SETUP_SQPOLL are both enabled, app don't
>> need to enter io_cqring_wait too. If I misunderstand, please give
>> me some advise.
> 
> The logic should be as follows:
> 
> flags			method
> ---------------------------------------------
> 0			io_cqring_wait()
> IOPOLL			io_iopoll_check()
> IOPOLL | SQPOLL		io_cqring_wait()
> SQPOLL			io_cqring_wait()
> 
> The reasoning being that we do want to enter cqring_wait() for SQPOLL,
> as the application may want to wait for completions. Even with IOPOLL
> set. As far as I can tell, the current code is correct, as long as we
> know SQPOLL will always poll for events for us.
Yes, agree.

Regards,
Xiaoguang Wang

> 
> So I'm curious why you think your patch is needed? Leaving it below and
> CC'ing Xiaoguang, who made the most recent change, so he can comment.
> 
>> Signed-off-by Liming Wu <19092205@suning.com>
>> ---
>>   io_uring.c | 11 ++++++-----
>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/io_uring.c b/io_uring.c
>> index b12d33b..36e884f 100644
>> --- a/io_uring.c
>> +++ b/io_uring.c
>> @@ -7418,11 +7418,12 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
>>   		 * polling again, they can rely on io_sq_thread to do polling
>>   		 * work, which can reduce cpu usage and uring_lock contention.
>>   		 */
>> -		if (ctx->flags & IORING_SETUP_IOPOLL &&
>> -		    !(ctx->flags & IORING_SETUP_SQPOLL)) {
>> -			ret = io_iopoll_check(ctx, &nr_events, min_complete);
>> -		} else {
>> -			ret = io_cqring_wait(ctx, min_complete, sig, sigsz);
>> +		if (!(ctx->flags & IORING_SETUP_SQPOLL)) {
>> +		    if (ctx->flags & IORING_SETUP_IOPOLL) {
>> +		    	ret = io_iopoll_check(ctx, &nr_events, min_complete);
>> +		    } else {
>> +		    	ret = io_cqring_wait(ctx, min_complete, sig, sigsz);
>> +		    }
>>   		}
>>   	}
>>   
>>
> 
> 

Re: [PATCH] io_uring:IORING_SETUP_SQPOLL don't need to enter io_cqring_wait

[Xiaoguang Wang]

hi,

> From: Liming Wu <19092205@suning.com>
> 
> When SETUP_IOPOLL and SETUP_SQPOLL are both enabled, app don't
> need to enter io_cqring_wait too. If I misunderstand, please give
> me some advise.
> 
> Signed-off-by Liming Wu <19092205@suning.com>
> ---
>   io_uring.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/io_uring.c b/io_uring.c
> index b12d33b..36e884f 100644
> --- a/io_uring.c
> +++ b/io_uring.c
> @@ -7418,11 +7418,12 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
>   		 * polling again, they can rely on io_sq_thread to do polling
>   		 * work, which can reduce cpu usage and uring_lock contention.
>   		 */
> -		if (ctx->flags & IORING_SETUP_IOPOLL &&
> -		    !(ctx->flags & IORING_SETUP_SQPOLL)) {
> -			ret = io_iopoll_check(ctx, &nr_events, min_complete);
> -		} else {
> -			ret = io_cqring_wait(ctx, min_complete, sig, sigsz);
> +		if (!(ctx->flags & IORING_SETUP_SQPOLL)) {
> +		    if (ctx->flags & IORING_SETUP_IOPOLL) {
> +		    	ret = io_iopoll_check(ctx, &nr_events, min_complete);
> +		    } else {
> +		    	ret = io_cqring_wait(ctx, min_complete, sig, sigsz);
> +		    }
>   		}Indeed I had checked your patch yesterday, and also did not understand why you have
above modifications. For your codes, if IORING_SETUP_SQPOLL is enabed, we'll do
nothing for IORING_ENTER_GETEVENTS, I think it's not correct.

In patch's commit message, I think you should explain more why you make such
changes, what's the benefit? You can have a look at my previous patch:
     io_uring: io_uring_enter(2) don't poll while SETUP_IOPOLL|SETUP_SQPOLL enabled

Finally, seems like that your patch did't pass checkpatch.pl, you should checkpatch
before sending patches:)

Regards,
Xiaoguang Wang

>   	}
>   
>