Re: [PATCH] io_uring: save repeated issue_flags

[Jens Axboe <axboe@kernel.dk>]

On 12/5/23 2:55 PM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> No need to rebuild the issue_flags on every IO: they're always the same.
> 
> Suggested-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  include/linux/io_uring_types.h |  1 +
>  io_uring/io_uring.c            | 15 ++++++++++++---
>  io_uring/uring_cmd.c           |  8 +-------
>  3 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
> index bebab36abce89..dd192d828f463 100644
> --- a/include/linux/io_uring_types.h
> +++ b/include/linux/io_uring_types.h
> @@ -228,6 +228,7 @@ struct io_ring_ctx {
>  	/* const or read-mostly hot data */
>  	struct {
>  		unsigned int		flags;
> +		unsigned int		issue_flags;
>  		unsigned int		drain_next: 1;
>  		unsigned int		restricted: 1;
>  		unsigned int		off_timeout_used: 1;
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 1d254f2c997de..a338e3660ecb8 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -3975,11 +3975,20 @@ static __cold int io_uring_create(unsigned entries, struct io_uring_params *p,
>  	 * 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))
> -		ctx->syscall_iopoll = 1;
> +	if (ctx->flags & IORING_SETUP_IOPOLL) {
> +		ctx->issue_flags |= IO_URING_F_SQE128;
> +		if (!(ctx->flags & IORING_SETUP_SQPOLL))
> +			ctx->syscall_iopoll = 1;
> +	}

This does not look correct?

>  	ctx->compat = in_compat_syscall();
> +	if (ctx->compat)
> +		ctx->issue_flags |= IO_URING_F_COMPAT;
> +	if (ctx->flags & IORING_SETUP_SQE128)
> +		ctx->issue_flags |= IO_URING_F_SQE128;
> +	if (ctx->flags & IORING_SETUP_CQE32)
> +		ctx->issue_flags |= IO_URING_F_CQE32;
> +
>  	if (!ns_capable_noaudit(&init_user_ns, CAP_IPC_LOCK))
>  		ctx->user = get_uid(current_user());
>  
> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> index 8a38b9f75d841..dbc0bfbfd0f05 100644
> --- a/io_uring/uring_cmd.c
> +++ b/io_uring/uring_cmd.c
> @@ -158,19 +158,13 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>  	if (ret)
>  		return ret;
>  
> -	if (ctx->flags & IORING_SETUP_SQE128)
> -		issue_flags |= IO_URING_F_SQE128;
> -	if (ctx->flags & IORING_SETUP_CQE32)
> -		issue_flags |= IO_URING_F_CQE32;
> -	if (ctx->compat)
> -		issue_flags |= IO_URING_F_COMPAT;
>  	if (ctx->flags & IORING_SETUP_IOPOLL) {
>  		if (!file->f_op->uring_cmd_iopoll)
>  			return -EOPNOTSUPP;
> -		issue_flags |= IO_URING_F_IOPOLL;
>  		req->iopoll_completed = 0;
>  	}
>  
> +	issue_flags |= ctx->issue_flags;
>  	ret = file->f_op->uring_cmd(ioucmd, issue_flags);
>  	if (ret == -EAGAIN) {
>  		if (!req_has_async_data(req)) {

I obviously like this idea, but it should be accompanied by getting rid
of ->compat and ->syscall_iopoll in the ctx as well?

-- 
Jens Axboe