diff mbox series

io_uring/napi: enable even with a timeout of 0

Message ID c7210193-a850-465c-bee2-ade5b36b4e2d@kernel.dk (mailing list archive)
State New
Headers show
Series io_uring/napi: enable even with a timeout of 0 | expand

Commit Message

Jens Axboe Feb. 15, 2024, 10:32 p.m. UTC 
1 usec is not as short as it used to be, and it makes sense to allow 0
for a busy poll timeout - this means just do one loop to check if we
have anything available. Add a separate ->napi_enabled to check if napi
has been enabled or not.

While at it, move the writing of the ctx napi values after we've copied
the old values back to userspace. This ensures that if the call fails,
we'll be in the same state as we were before, rather than some
indeterminate state.

Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

Comments

David Wei Feb. 15, 2024, 10:37 p.m. UTC | #1 
On 2024-02-15 15:32, Jens Axboe wrote:
> 1 usec is not as short as it used to be, and it makes sense to allow 0
> for a busy poll timeout - this means just do one loop to check if we
> have anything available. Add a separate ->napi_enabled to check if napi
> has been enabled or not.
> 
> While at it, move the writing of the ctx napi values after we've copied
> the old values back to userspace. This ensures that if the call fails,
> we'll be in the same state as we were before, rather than some
> indeterminate state.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> ---
> 
> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
> index 4fe7af8a4907..bd7071aeec5d 100644
> --- a/include/linux/io_uring_types.h
> +++ b/include/linux/io_uring_types.h
> @@ -420,6 +420,7 @@ struct io_ring_ctx {
>  	/* napi busy poll default timeout */
>  	unsigned int		napi_busy_poll_to;
>  	bool			napi_prefer_busy_poll;
> +	bool			napi_enabled;
>  
>  	DECLARE_HASHTABLE(napi_ht, 4);
>  #endif
> diff --git a/io_uring/napi.c b/io_uring/napi.c
> index b234adda7dfd..e653927a376e 100644
> --- a/io_uring/napi.c
> +++ b/io_uring/napi.c
> @@ -227,12 +227,12 @@ int io_register_napi(struct io_ring_ctx *ctx, void __user *arg)
>  	if (napi.pad[0] || napi.pad[1] || napi.pad[2] || napi.resv)
>  		return -EINVAL;
>  
> -	WRITE_ONCE(ctx->napi_busy_poll_to, napi.busy_poll_to);
> -	WRITE_ONCE(ctx->napi_prefer_busy_poll, !!napi.prefer_busy_poll);
> -
>  	if (copy_to_user(arg, &curr, sizeof(curr)))
>  		return -EFAULT;
>  
> +	WRITE_ONCE(ctx->napi_busy_poll_to, napi.busy_poll_to);
> +	WRITE_ONCE(ctx->napi_prefer_busy_poll, !!napi.prefer_busy_poll);
> +	WRITE_ONCE(ctx->napi_enabled, true);
>  	return 0;
>  }
>  
> @@ -256,6 +256,7 @@ int io_unregister_napi(struct io_ring_ctx *ctx, void __user *arg)
>  
>  	WRITE_ONCE(ctx->napi_busy_poll_to, 0);
>  	WRITE_ONCE(ctx->napi_prefer_busy_poll, false);
> +	WRITE_ONCE(ctx->napi_enabled, true);

Should this be false?

>  	return 0;
>  }
>  
> @@ -300,7 +301,7 @@ void __io_napi_busy_loop(struct io_ring_ctx *ctx, struct io_wait_queue *iowq)
>  {
>  	iowq->napi_prefer_busy_poll = READ_ONCE(ctx->napi_prefer_busy_poll);
>  
> -	if (!(ctx->flags & IORING_SETUP_SQPOLL) && iowq->napi_busy_poll_to)
> +	if (!(ctx->flags & IORING_SETUP_SQPOLL) && ctx->napi_enabled)
>  		io_napi_blocking_busy_loop(ctx, iowq);
>  }
>
Jens Axboe Feb. 15, 2024, 10:37 p.m. UTC | #2 
On 2/15/24 3:37 PM, David Wei wrote:
> On 2024-02-15 15:32, Jens Axboe wrote:
>> 1 usec is not as short as it used to be, and it makes sense to allow 0
>> for a busy poll timeout - this means just do one loop to check if we
>> have anything available. Add a separate ->napi_enabled to check if napi
>> has been enabled or not.
>>
>> While at it, move the writing of the ctx napi values after we've copied
>> the old values back to userspace. This ensures that if the call fails,
>> we'll be in the same state as we were before, rather than some
>> indeterminate state.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> ---
>>
>> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
>> index 4fe7af8a4907..bd7071aeec5d 100644
>> --- a/include/linux/io_uring_types.h
>> +++ b/include/linux/io_uring_types.h
>> @@ -420,6 +420,7 @@ struct io_ring_ctx {
>>  	/* napi busy poll default timeout */
>>  	unsigned int		napi_busy_poll_to;
>>  	bool			napi_prefer_busy_poll;
>> +	bool			napi_enabled;
>>  
>>  	DECLARE_HASHTABLE(napi_ht, 4);
>>  #endif
>> diff --git a/io_uring/napi.c b/io_uring/napi.c
>> index b234adda7dfd..e653927a376e 100644
>> --- a/io_uring/napi.c
>> +++ b/io_uring/napi.c
>> @@ -227,12 +227,12 @@ int io_register_napi(struct io_ring_ctx *ctx, void __user *arg)
>>  	if (napi.pad[0] || napi.pad[1] || napi.pad[2] || napi.resv)
>>  		return -EINVAL;
>>  
>> -	WRITE_ONCE(ctx->napi_busy_poll_to, napi.busy_poll_to);
>> -	WRITE_ONCE(ctx->napi_prefer_busy_poll, !!napi.prefer_busy_poll);
>> -
>>  	if (copy_to_user(arg, &curr, sizeof(curr)))
>>  		return -EFAULT;
>>  
>> +	WRITE_ONCE(ctx->napi_busy_poll_to, napi.busy_poll_to);
>> +	WRITE_ONCE(ctx->napi_prefer_busy_poll, !!napi.prefer_busy_poll);
>> +	WRITE_ONCE(ctx->napi_enabled, true);
>>  	return 0;
>>  }
>>  
>> @@ -256,6 +256,7 @@ int io_unregister_napi(struct io_ring_ctx *ctx, void __user *arg)
>>  
>>  	WRITE_ONCE(ctx->napi_busy_poll_to, 0);
>>  	WRITE_ONCE(ctx->napi_prefer_busy_poll, false);
>> +	WRITE_ONCE(ctx->napi_enabled, true);
> 
> Should this be false?

It should indeed... Updated, thanks!
diff mbox series

Patch

diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index 4fe7af8a4907..bd7071aeec5d 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -420,6 +420,7 @@  struct io_ring_ctx {
 	/* napi busy poll default timeout */
 	unsigned int		napi_busy_poll_to;
 	bool			napi_prefer_busy_poll;
+	bool			napi_enabled;
 
 	DECLARE_HASHTABLE(napi_ht, 4);
 #endif
diff --git a/io_uring/napi.c b/io_uring/napi.c
index b234adda7dfd..e653927a376e 100644
--- a/io_uring/napi.c
+++ b/io_uring/napi.c
@@ -227,12 +227,12 @@  int io_register_napi(struct io_ring_ctx *ctx, void __user *arg)
 	if (napi.pad[0] || napi.pad[1] || napi.pad[2] || napi.resv)
 		return -EINVAL;
 
-	WRITE_ONCE(ctx->napi_busy_poll_to, napi.busy_poll_to);
-	WRITE_ONCE(ctx->napi_prefer_busy_poll, !!napi.prefer_busy_poll);
-
 	if (copy_to_user(arg, &curr, sizeof(curr)))
 		return -EFAULT;
 
+	WRITE_ONCE(ctx->napi_busy_poll_to, napi.busy_poll_to);
+	WRITE_ONCE(ctx->napi_prefer_busy_poll, !!napi.prefer_busy_poll);
+	WRITE_ONCE(ctx->napi_enabled, true);
 	return 0;
 }
 
@@ -256,6 +256,7 @@  int io_unregister_napi(struct io_ring_ctx *ctx, void __user *arg)
 
 	WRITE_ONCE(ctx->napi_busy_poll_to, 0);
 	WRITE_ONCE(ctx->napi_prefer_busy_poll, false);
+	WRITE_ONCE(ctx->napi_enabled, true);
 	return 0;
 }
 
@@ -300,7 +301,7 @@  void __io_napi_busy_loop(struct io_ring_ctx *ctx, struct io_wait_queue *iowq)
 {
 	iowq->napi_prefer_busy_poll = READ_ONCE(ctx->napi_prefer_busy_poll);
 
-	if (!(ctx->flags & IORING_SETUP_SQPOLL) && iowq->napi_busy_poll_to)
+	if (!(ctx->flags & IORING_SETUP_SQPOLL) && ctx->napi_enabled)
 		io_napi_blocking_busy_loop(ctx, iowq);
 }