Re: [PATCH v13 4/7] io-uring: add napi busy poll support

From: Stefan Roesch <shr@devkernel.io>
To: Jens Axboe <axboe@kernel.dk>
Cc: io-uring@vger.kernel.org, kernel-team@fb.com,
	ammarfaizi2@gnuweeb.org, netdev@vger.kernel.org, kuba@kernel.org,
	olivier@trillion01.com
Subject: Re: [PATCH v13 4/7] io-uring: add napi busy poll support
Date: Fri, 19 May 2023 16:11:30 -0700	[thread overview]
Message-ID: <qvqwr0rbx6f5.fsf@devbig1114.prn1.facebook.com> (raw)
In-Reply-To: <4b05488a-6a12-2f23-f490-79dcc2bc5d59@kernel.dk>

Jens Axboe <axboe@kernel.dk> writes:

> On 5/18/23 3:17?PM, Stefan Roesch wrote:
>> diff --git a/io_uring/poll.c b/io_uring/poll.c
>> index c90e47dc1e29..0284849793bb 100644
>> --- a/io_uring/poll.c
>> +++ b/io_uring/poll.c
>> @@ -15,6 +15,7 @@
>>
>>  #include "io_uring.h"
>>  #include "refs.h"
>> +#include "napi.h"
>>  #include "opdef.h"
>>  #include "kbuf.h"
>>  #include "poll.h"
>> @@ -631,6 +632,7 @@ static int __io_arm_poll_handler(struct io_kiocb *req,
>>  		__io_poll_execute(req, mask);
>>  		return 0;
>>  	}
>> +	io_napi_add(req);
>>
>>  	if (ipt->owning) {
>>  		/*
>
> One thing that bothers me a bit here is that we then do:
>
> static inline void io_napi_add(struct io_kiocb *req)
> {
> 	struct io_ring_ctx *ctx = req->ctx;
>
> 	if (!READ_ONCE(ctx->napi_busy_poll_to))
> 		return;
>
> 	__io_napi_add(ctx, req->file);
> }
>
> which will do __io_napi_add() for any file type, even though we really
> just want sockets here. I initially thought we could add a fast flag for
> the type (like we do for IS_REG), but I think we can just do this
> incremental and call it good enough.
>
> Unfortunately sock_from_file() is also a function call... But not a huge
> problem.
>
>
> diff --git a/io_uring/napi.c b/io_uring/napi.c
> index 5790b2daf1d0..8ec016899539 100644
> --- a/io_uring/napi.c
> +++ b/io_uring/napi.c
> @@ -33,18 +33,13 @@ static struct io_napi_entry *io_napi_hash_find(struct hlist_head *hash_list,
>  	return NULL;
>  }
>
> -void __io_napi_add(struct io_ring_ctx *ctx, struct file *file)
> +void __io_napi_add(struct io_ring_ctx *ctx, struct socket *sock)
>  {
>  	struct hlist_head *hash_list;
>  	unsigned int napi_id;
> -	struct socket *sock;
>  	struct sock *sk;
>  	struct io_napi_entry *e;
>
> -	sock = sock_from_file(file);
> -	if (!sock)
> -		return;
> -
>  	sk = sock->sk;
>  	if (!sk)
>  		return;
> diff --git a/io_uring/napi.h b/io_uring/napi.h
> index 69c1970cbecc..08cee8f4c9d1 100644
> --- a/io_uring/napi.h
> +++ b/io_uring/napi.h
> @@ -15,7 +15,7 @@ void io_napi_free(struct io_ring_ctx *ctx);
>  int io_register_napi(struct io_ring_ctx *ctx, void __user *arg);
>  int io_unregister_napi(struct io_ring_ctx *ctx, void __user *arg);
>
> -void __io_napi_add(struct io_ring_ctx *ctx, struct file *file);
> +void __io_napi_add(struct io_ring_ctx *ctx, struct socket *sock);
>
>  void __io_napi_adjust_timeout(struct io_ring_ctx *ctx,
>  		struct io_wait_queue *iowq, struct timespec64 *ts);
> @@ -53,46 +53,51 @@ static inline void io_napi_busy_loop(struct io_ring_ctx *ctx,
>  static inline void io_napi_add(struct io_kiocb *req)
>  {
>  	struct io_ring_ctx *ctx = req->ctx;
> +	struct socket *sock;
>
>  	if (!READ_ONCE(ctx->napi_busy_poll_to))
>  		return;
>
> -	__io_napi_add(ctx, req->file);
> +	sock = sock_from_file(req->file);
> +	if (sock)
> +		__io_napi_add(ctx, sock);
>  }

I'll add the above changes to the next version.