Re: [PATCH 1/6] io_uring: expand main struct io_kiocb flags to 64-bits

[Pavel Begunkov <asml.silence@gmail.com>]

On 2/6/24 16:22, Jens Axboe wrote:
> We're out of space here, and none of the flags are easily reclaimable.
> Bump it to 64-bits and re-arrange the struct a bit to avoid gaps.
> 
> Add a specific bitwise type for the request flags, io_request_flags_t.
> This will help catch violations of casting this value to a smaller type
> on 32-bit archs, like unsigned int.
> 
> No functional changes intended in this patch.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>   include/linux/io_uring_types.h  | 87 ++++++++++++++++++---------------
>   include/trace/events/io_uring.h | 14 +++---
>   io_uring/filetable.h            |  2 +-
>   io_uring/io_uring.c             |  9 ++--
>   4 files changed, 60 insertions(+), 52 deletions(-)
> 
> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
> index 854ad67a5f70..5ac18b05d4ee 100644
> --- a/include/linux/io_uring_types.h
> +++ b/include/linux/io_uring_types.h
> @@ -428,7 +428,7 @@ struct io_tw_state {
>   	bool locked;
>   };
>   
> -enum {
> +enum io_req_flags {
>   	REQ_F_FIXED_FILE_BIT	= IOSQE_FIXED_FILE_BIT,
>   	REQ_F_IO_DRAIN_BIT	= IOSQE_IO_DRAIN_BIT,
>   	REQ_F_LINK_BIT		= IOSQE_IO_LINK_BIT,
> @@ -468,70 +468,73 @@ enum {
>   	__REQ_F_LAST_BIT,
>   };
>   
> +typedef enum io_req_flags __bitwise io_req_flags_t;
> +#define IO_REQ_FLAG(bitno)	((__force io_req_flags_t) BIT_ULL((bitno)))
> +
>   enum {
>   	/* ctx owns file */
> -	REQ_F_FIXED_FILE	= BIT(REQ_F_FIXED_FILE_BIT),
> +	REQ_F_FIXED_FILE	= IO_REQ_FLAG(REQ_F_FIXED_FILE_BIT),
>   	/* drain existing IO first */
> -	REQ_F_IO_DRAIN		= BIT(REQ_F_IO_DRAIN_BIT),
> +	REQ_F_IO_DRAIN		= IO_REQ_FLAG(REQ_F_IO_DRAIN_BIT),
>   	/* linked sqes */
> -	REQ_F_LINK		= BIT(REQ_F_LINK_BIT),
> +	REQ_F_LINK		= IO_REQ_FLAG(REQ_F_LINK_BIT),
>   	/* doesn't sever on completion < 0 */
> -	REQ_F_HARDLINK		= BIT(REQ_F_HARDLINK_BIT),
> +	REQ_F_HARDLINK		= IO_REQ_FLAG(REQ_F_HARDLINK_BIT),
>   	/* IOSQE_ASYNC */
> -	REQ_F_FORCE_ASYNC	= BIT(REQ_F_FORCE_ASYNC_BIT),
> +	REQ_F_FORCE_ASYNC	= IO_REQ_FLAG(REQ_F_FORCE_ASYNC_BIT),
>   	/* IOSQE_BUFFER_SELECT */
> -	REQ_F_BUFFER_SELECT	= BIT(REQ_F_BUFFER_SELECT_BIT),
> +	REQ_F_BUFFER_SELECT	= IO_REQ_FLAG(REQ_F_BUFFER_SELECT_BIT),
>   	/* IOSQE_CQE_SKIP_SUCCESS */
> -	REQ_F_CQE_SKIP		= BIT(REQ_F_CQE_SKIP_BIT),
> +	REQ_F_CQE_SKIP		= IO_REQ_FLAG(REQ_F_CQE_SKIP_BIT),
>   
>   	/* fail rest of links */
> -	REQ_F_FAIL		= BIT(REQ_F_FAIL_BIT),
> +	REQ_F_FAIL		= IO_REQ_FLAG(REQ_F_FAIL_BIT),
>   	/* on inflight list, should be cancelled and waited on exit reliably */
> -	REQ_F_INFLIGHT		= BIT(REQ_F_INFLIGHT_BIT),
> +	REQ_F_INFLIGHT		= IO_REQ_FLAG(REQ_F_INFLIGHT_BIT),
>   	/* read/write uses file position */
> -	REQ_F_CUR_POS		= BIT(REQ_F_CUR_POS_BIT),
> +	REQ_F_CUR_POS		= IO_REQ_FLAG(REQ_F_CUR_POS_BIT),
>   	/* must not punt to workers */
> -	REQ_F_NOWAIT		= BIT(REQ_F_NOWAIT_BIT),
> +	REQ_F_NOWAIT		= IO_REQ_FLAG(REQ_F_NOWAIT_BIT),
>   	/* has or had linked timeout */
> -	REQ_F_LINK_TIMEOUT	= BIT(REQ_F_LINK_TIMEOUT_BIT),
> +	REQ_F_LINK_TIMEOUT	= IO_REQ_FLAG(REQ_F_LINK_TIMEOUT_BIT),
>   	/* needs cleanup */
> -	REQ_F_NEED_CLEANUP	= BIT(REQ_F_NEED_CLEANUP_BIT),
> +	REQ_F_NEED_CLEANUP	= IO_REQ_FLAG(REQ_F_NEED_CLEANUP_BIT),
>   	/* already went through poll handler */
> -	REQ_F_POLLED		= BIT(REQ_F_POLLED_BIT),
> +	REQ_F_POLLED		= IO_REQ_FLAG(REQ_F_POLLED_BIT),
>   	/* buffer already selected */
> -	REQ_F_BUFFER_SELECTED	= BIT(REQ_F_BUFFER_SELECTED_BIT),
> +	REQ_F_BUFFER_SELECTED	= IO_REQ_FLAG(REQ_F_BUFFER_SELECTED_BIT),
>   	/* buffer selected from ring, needs commit */
> -	REQ_F_BUFFER_RING	= BIT(REQ_F_BUFFER_RING_BIT),
> +	REQ_F_BUFFER_RING	= IO_REQ_FLAG(REQ_F_BUFFER_RING_BIT),
>   	/* caller should reissue async */
> -	REQ_F_REISSUE		= BIT(REQ_F_REISSUE_BIT),
> +	REQ_F_REISSUE		= IO_REQ_FLAG(REQ_F_REISSUE_BIT),
>   	/* supports async reads/writes */
> -	REQ_F_SUPPORT_NOWAIT	= BIT(REQ_F_SUPPORT_NOWAIT_BIT),
> +	REQ_F_SUPPORT_NOWAIT	= IO_REQ_FLAG(REQ_F_SUPPORT_NOWAIT_BIT),
>   	/* regular file */
> -	REQ_F_ISREG		= BIT(REQ_F_ISREG_BIT),
> +	REQ_F_ISREG		= IO_REQ_FLAG(REQ_F_ISREG_BIT),
>   	/* has creds assigned */
> -	REQ_F_CREDS		= BIT(REQ_F_CREDS_BIT),
> +	REQ_F_CREDS		= IO_REQ_FLAG(REQ_F_CREDS_BIT),
>   	/* skip refcounting if not set */
> -	REQ_F_REFCOUNT		= BIT(REQ_F_REFCOUNT_BIT),
> +	REQ_F_REFCOUNT		= IO_REQ_FLAG(REQ_F_REFCOUNT_BIT),
>   	/* there is a linked timeout that has to be armed */
> -	REQ_F_ARM_LTIMEOUT	= BIT(REQ_F_ARM_LTIMEOUT_BIT),
> +	REQ_F_ARM_LTIMEOUT	= IO_REQ_FLAG(REQ_F_ARM_LTIMEOUT_BIT),
>   	/* ->async_data allocated */
> -	REQ_F_ASYNC_DATA	= BIT(REQ_F_ASYNC_DATA_BIT),
> +	REQ_F_ASYNC_DATA	= IO_REQ_FLAG(REQ_F_ASYNC_DATA_BIT),
>   	/* don't post CQEs while failing linked requests */
> -	REQ_F_SKIP_LINK_CQES	= BIT(REQ_F_SKIP_LINK_CQES_BIT),
> +	REQ_F_SKIP_LINK_CQES	= IO_REQ_FLAG(REQ_F_SKIP_LINK_CQES_BIT),
>   	/* single poll may be active */
> -	REQ_F_SINGLE_POLL	= BIT(REQ_F_SINGLE_POLL_BIT),
> +	REQ_F_SINGLE_POLL	= IO_REQ_FLAG(REQ_F_SINGLE_POLL_BIT),
>   	/* double poll may active */
> -	REQ_F_DOUBLE_POLL	= BIT(REQ_F_DOUBLE_POLL_BIT),
> +	REQ_F_DOUBLE_POLL	= IO_REQ_FLAG(REQ_F_DOUBLE_POLL_BIT),
>   	/* request has already done partial IO */
> -	REQ_F_PARTIAL_IO	= BIT(REQ_F_PARTIAL_IO_BIT),
> +	REQ_F_PARTIAL_IO	= IO_REQ_FLAG(REQ_F_PARTIAL_IO_BIT),
>   	/* fast poll multishot mode */
> -	REQ_F_APOLL_MULTISHOT	= BIT(REQ_F_APOLL_MULTISHOT_BIT),
> +	REQ_F_APOLL_MULTISHOT	= IO_REQ_FLAG(REQ_F_APOLL_MULTISHOT_BIT),
>   	/* recvmsg special flag, clear EPOLLIN */
> -	REQ_F_CLEAR_POLLIN	= BIT(REQ_F_CLEAR_POLLIN_BIT),
> +	REQ_F_CLEAR_POLLIN	= IO_REQ_FLAG(REQ_F_CLEAR_POLLIN_BIT),
>   	/* hashed into ->cancel_hash_locked, protected by ->uring_lock */
> -	REQ_F_HASH_LOCKED	= BIT(REQ_F_HASH_LOCKED_BIT),
> +	REQ_F_HASH_LOCKED	= IO_REQ_FLAG(REQ_F_HASH_LOCKED_BIT),
>   	/* don't use lazy poll wake for this request */
> -	REQ_F_POLL_NO_LAZY	= BIT(REQ_F_POLL_NO_LAZY_BIT),
> +	REQ_F_POLL_NO_LAZY	= IO_REQ_FLAG(REQ_F_POLL_NO_LAZY_BIT),
>   };
>   
>   typedef void (*io_req_tw_func_t)(struct io_kiocb *req, struct io_tw_state *ts);
> @@ -592,15 +595,14 @@ struct io_kiocb {
>   	 * and after selection it points to the buffer ID itself.
>   	 */
>   	u16				buf_index;
> -	unsigned int			flags;
>   
> -	struct io_cqe			cqe;

With the current layout the min number of lines we touch per
request is 2 (including the op specific 64B), that's includes
setting up cqe at init and using it for completing. Moving cqe
down makes it 3.

> +	atomic_t			refs;

We're pulling it refs, which is not touched at all in the hot
path. Even if there's a hole I'd argue it's better to leave it
at the end.

> +
> +	io_req_flags_t			flags;
>   
>   	struct io_ring_ctx		*ctx;
>   	struct task_struct		*task;
>   
> -	struct io_rsrc_node		*rsrc_node;

It's used in hot paths, registered buffers/files, would be
unfortunate to move it to the next line.

> -
>   	union {
>   		/* store used ubuf, so we can prevent reloading */
>   		struct io_mapped_ubuf	*imu;
> @@ -615,18 +617,23 @@ struct io_kiocb {
>   		struct io_buffer_list	*buf_list;
>   	};
>   
> +	/* for polled requests, i.e. IORING_OP_POLL_ADD and async armed poll */
> +	struct hlist_node		hash_node;
> +

And we're pulling hash_node into the hottest line, which is
used only when we arm a poll and remove poll. So, it's mostly
for networking, sends wouldn't use it much, and multishots
wouldn't normally touch it.

As for ideas how to find space:
1) iopoll_completed completed can be converted to flags2

2) REQ_F_{SINGLE,DOUBLE}_POLL is a weird duplication. Can
probably be combined into one flag, or removed at all.
Again, sends are usually not so poll heavy and the hot
path for recv is multishot.

3) we can probably move req->task down and replace it with

get_task() {
     if (req->ctx->flags & DEFER_TASKRUN)
         task = ctx->submitter_task;
     else
         task = req->task;
}

The most common user of it -- task_work_add -- already
checks the flag and has separate paths, and
init/completion paths can be optimised.


>   	union {
>   		/* used by request caches, completion batching and iopoll */
>   		struct io_wq_work_node	comp_list;
>   		/* cache ->apoll->events */
>   		__poll_t apoll_events;
>   	};
> -	atomic_t			refs;
> -	atomic_t			poll_refs;
> +
> +	struct io_rsrc_node		*rsrc_node;
> +
> +	struct io_cqe			cqe;
> +
>   	struct io_task_work		io_task_work;
> +	atomic_t			poll_refs;
>   	unsigned			nr_tw;
> -	/* for polled requests, i.e. IORING_OP_POLL_ADD and async armed poll */
> -	struct hlist_node		hash_node;
>   	/* internal polling, see IORING_FEAT_FAST_POLL */
>   	struct async_poll		*apoll;
>   	/* opcode allocated if it needs to store data for async defer */
> diff --git a/include/trace/events/io_uring.h b/include/trace/events/io_uring.h
> index 69454f1f98b0..3d7704a52b73 100644
> --- a/include/trace/events/io_uring.h
> +++ b/include/trace/events/io_uring.h
> @@ -148,7 +148,7 @@ TRACE_EVENT(io_uring_queue_async_work,
>   		__field(  void *,			req		)
>   		__field(  u64,				user_data	)
>   		__field(  u8,				opcode		)
> -		__field(  unsigned int,			flags		)
> +		__field(  io_req_flags_t,		flags		)
>   		__field(  struct io_wq_work *,		work		)
>   		__field(  int,				rw		)
>   
> @@ -167,10 +167,10 @@ TRACE_EVENT(io_uring_queue_async_work,
>   		__assign_str(op_str, io_uring_get_opcode(req->opcode));
>   	),
>   
> -	TP_printk("ring %p, request %p, user_data 0x%llx, opcode %s, flags 0x%x, %s queue, work %p",
> +	TP_printk("ring %p, request %p, user_data 0x%llx, opcode %s, flags 0x%lx, %s queue, work %p",
>   		__entry->ctx, __entry->req, __entry->user_data,
> -		__get_str(op_str),
> -		__entry->flags, __entry->rw ? "hashed" : "normal", __entry->work)
> +		__get_str(op_str), (long) __entry->flags,
> +		__entry->rw ? "hashed" : "normal", __entry->work)
>   );
>   
>   /**
> @@ -378,7 +378,7 @@ TRACE_EVENT(io_uring_submit_req,
>   		__field(  void *,		req		)
>   		__field(  unsigned long long,	user_data	)
>   		__field(  u8,			opcode		)
> -		__field(  u32,			flags		)
> +		__field(  io_req_flags_t,	flags		)
>   		__field(  bool,			sq_thread	)
>   
>   		__string( op_str, io_uring_get_opcode(req->opcode) )
> @@ -395,10 +395,10 @@ TRACE_EVENT(io_uring_submit_req,
>   		__assign_str(op_str, io_uring_get_opcode(req->opcode));
>   	),
>   
> -	TP_printk("ring %p, req %p, user_data 0x%llx, opcode %s, flags 0x%x, "
> +	TP_printk("ring %p, req %p, user_data 0x%llx, opcode %s, flags 0x%lx, "
>   		  "sq_thread %d", __entry->ctx, __entry->req,
>   		  __entry->user_data, __get_str(op_str),
> -		  __entry->flags, __entry->sq_thread)
> +		  (long) __entry->flags, __entry->sq_thread)
>   );
>   
>   /*
> diff --git a/io_uring/filetable.h b/io_uring/filetable.h
> index b47adf170c31..b2435c4dca1f 100644
> --- a/io_uring/filetable.h
> +++ b/io_uring/filetable.h
> @@ -17,7 +17,7 @@ int io_fixed_fd_remove(struct io_ring_ctx *ctx, unsigned int offset);
>   int io_register_file_alloc_range(struct io_ring_ctx *ctx,
>   				 struct io_uring_file_index_range __user *arg);
>   
> -unsigned int io_file_get_flags(struct file *file);
> +io_req_flags_t io_file_get_flags(struct file *file);
>   
>   static inline void io_file_bitmap_clear(struct io_file_table *table, int bit)
>   {
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index cd9a137ad6ce..360a7ee41d3a 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -1768,9 +1768,9 @@ static void io_iopoll_req_issued(struct io_kiocb *req, unsigned int issue_flags)
>   	}
>   }
>   
> -unsigned int io_file_get_flags(struct file *file)
> +io_req_flags_t io_file_get_flags(struct file *file)
>   {
> -	unsigned int res = 0;
> +	io_req_flags_t res = 0;
>   
>   	if (S_ISREG(file_inode(file)->i_mode))
>   		res |= REQ_F_ISREG;
> @@ -2171,7 +2171,8 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
>   	/* req is partially pre-initialised, see io_preinit_req() */
>   	req->opcode = opcode = READ_ONCE(sqe->opcode);
>   	/* same numerical values with corresponding REQ_F_*, safe to copy */
> -	req->flags = sqe_flags = READ_ONCE(sqe->flags);
> +	sqe_flags = READ_ONCE(sqe->flags);
> +	req->flags = (io_req_flags_t) sqe_flags;
>   	req->cqe.user_data = READ_ONCE(sqe->user_data);
>   	req->file = NULL;
>   	req->rsrc_node = NULL;
> @@ -4153,7 +4154,7 @@ static int __init io_uring_init(void)
>   	BUILD_BUG_ON(SQE_COMMON_FLAGS >= (1 << 8));
>   	BUILD_BUG_ON((SQE_VALID_FLAGS | SQE_COMMON_FLAGS) != SQE_VALID_FLAGS);
>   
> -	BUILD_BUG_ON(__REQ_F_LAST_BIT > 8 * sizeof(int));
> +	BUILD_BUG_ON(__REQ_F_LAST_BIT > 8 * sizeof(u64));
>   
>   	BUILD_BUG_ON(sizeof(atomic_t) != sizeof(u32));
>   

-- 
Pavel Begunkov