[PATCHSET 0/2] Cleanup alloc cache init_once handling

[PATCHSET 0/2] Cleanup alloc cache init_once handling

[Jens Axboe]

Hi,

A minor prep patch cleaning up some confusion on types for uring_cmd,
which don't matter now, but will after patch 2. Patch 2 gets rid of
the init_once, and has the cache init functions pass in the number of
bytes to clear for a fresh allocation.

 include/linux/io_uring/cmd.h   |  3 ++-
 include/linux/io_uring_types.h |  3 ++-
 io_uring/alloc_cache.h         | 30 +++++++++++++++++++++---------
 io_uring/futex.c               |  4 ++--
 io_uring/io_uring.c            | 13 ++++++++-----
 io_uring/io_uring.h            |  5 ++---
 io_uring/net.c                 | 11 +----------
 io_uring/net.h                 |  7 +++++--
 io_uring/poll.c                |  2 +-
 io_uring/rw.c                  | 10 +---------
 io_uring/rw.h                  |  5 ++++-
 io_uring/uring_cmd.c           | 16 ++++------------
 12 files changed, 53 insertions(+), 56 deletions(-)

-- 
Jens Axboe

[PATCH 1/2] io_uring/uring_cmd: cleanup struct io_uring_cmd_data layout

[Jens Axboe]

A few spots in uring_cmd assume that the SQEs copied are always at the
start of the structure, and hence mix req->async_data and the struct
itself.

Clean that up and use the proper indices.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 io_uring/uring_cmd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index 3993c9339ac7..6a63ec4b5445 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -192,8 +192,8 @@ static int io_uring_cmd_prep_setup(struct io_kiocb *req,
 		return 0;
 	}
 
-	memcpy(req->async_data, sqe, uring_sqe_size(req->ctx));
-	ioucmd->sqe = req->async_data;
+	memcpy(cache->sqes, sqe, uring_sqe_size(req->ctx));
+	ioucmd->sqe = cache->sqes;
 	return 0;
 }
 
@@ -260,7 +260,7 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
 		struct io_uring_cmd_data *cache = req->async_data;
 
 		if (ioucmd->sqe != (void *) cache)
-			memcpy(cache, ioucmd->sqe, uring_sqe_size(req->ctx));
+			memcpy(cache->sqes, ioucmd->sqe, uring_sqe_size(req->ctx));
 		return -EAGAIN;
 	} else if (ret == -EIOCBQUEUED) {
 		return -EIOCBQUEUED;
-- 
2.47.2

[PATCH 2/2] io_uring: get rid of alloc cache init_once handling

[Jens Axboe]

init_once is called when an object doesn't come from the cache, and
hence needs initial clearing of certain members. While the whole
struct could get cleared by memset() in that case, a few of the cache
members are large enough that this may cause unnecessary overhead if
the caches used aren't large enough to satisfy the workload. For those
cases, some churn of kmalloc+kfree is to be expected.

Ensure that the 3 users that need clearing put the members they need
cleared at the start of the struct, and place an empty placeholder
'init' member so that the cache initialization knows how much to
clear.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/io_uring/cmd.h   |  3 ++-
 include/linux/io_uring_types.h |  3 ++-
 io_uring/alloc_cache.h         | 30 +++++++++++++++++++++---------
 io_uring/futex.c               |  4 ++--
 io_uring/io_uring.c            | 13 ++++++++-----
 io_uring/io_uring.h            |  5 ++---
 io_uring/net.c                 | 11 +----------
 io_uring/net.h                 |  7 +++++--
 io_uring/poll.c                |  2 +-
 io_uring/rw.c                  | 10 +---------
 io_uring/rw.h                  |  5 ++++-
 io_uring/uring_cmd.c           | 10 +---------
 12 files changed, 50 insertions(+), 53 deletions(-)

diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
index a3ce553413de..8d7746d9fd23 100644
--- a/include/linux/io_uring/cmd.h
+++ b/include/linux/io_uring/cmd.h
@@ -19,8 +19,9 @@ struct io_uring_cmd {
 };
 
 struct io_uring_cmd_data {
-	struct io_uring_sqe	sqes[2];
 	void			*op_data;
+	int			init[0];
+	struct io_uring_sqe	sqes[2];
 };
 
 static inline const void *io_uring_sqe_cmd(const struct io_uring_sqe *sqe)
diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index 623d8e798a11..3def525a1da3 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -222,7 +222,8 @@ struct io_alloc_cache {
 	void			**entries;
 	unsigned int		nr_cached;
 	unsigned int		max_cached;
-	size_t			elem_size;
+	unsigned int		elem_size;
+	unsigned int		init_clear;
 };
 
 struct io_ring_ctx {
diff --git a/io_uring/alloc_cache.h b/io_uring/alloc_cache.h
index a3a8cfec32ce..f57d5e13cbbb 100644
--- a/io_uring/alloc_cache.h
+++ b/io_uring/alloc_cache.h
@@ -23,35 +23,47 @@ static inline void *io_alloc_cache_get(struct io_alloc_cache *cache)
 	if (cache->nr_cached) {
 		void *entry = cache->entries[--cache->nr_cached];
 
+		/*
+		 * If KASAN is enabled, always clear the initial bytes that
+		 * must be zeroed post alloc, in case any of them overlap
+		 * with KASAN storage.
+		 */
+#if defined(CONFIG_KASAN)
 		kasan_mempool_unpoison_object(entry, cache->elem_size);
+		if (cache->init_clear)
+			memset(entry, 0, cache->init_clear);
+#endif
 		return entry;
 	}
 
 	return NULL;
 }
 
-static inline void *io_cache_alloc(struct io_alloc_cache *cache, gfp_t gfp,
-				   void (*init_once)(void *obj))
+static inline void *io_cache_alloc(struct io_alloc_cache *cache, gfp_t gfp)
 {
-	if (unlikely(!cache->nr_cached)) {
-		void *obj = kmalloc(cache->elem_size, gfp);
+	void *obj;
 
-		if (obj && init_once)
-			init_once(obj);
+	obj = io_alloc_cache_get(cache);
+	if (obj)
 		return obj;
-	}
-	return io_alloc_cache_get(cache);
+
+	obj = kmalloc(cache->elem_size, gfp);
+	if (obj && cache->init_clear)
+		memset(obj, 0, cache->init_clear);
+	return obj;
 }
 
 /* returns false if the cache was initialized properly */
 static inline bool io_alloc_cache_init(struct io_alloc_cache *cache,
-				       unsigned max_nr, size_t size)
+				       unsigned max_nr, unsigned int size,
+				       unsigned int init_bytes)
 {
 	cache->entries = kvmalloc_array(max_nr, sizeof(void *), GFP_KERNEL);
 	if (cache->entries) {
 		cache->nr_cached = 0;
 		cache->max_cached = max_nr;
 		cache->elem_size = size;
+		cache->init_clear = init_bytes;
 		return false;
 	}
 	return true;
diff --git a/io_uring/futex.c b/io_uring/futex.c
index 30139cc150f2..3159a2b7eeca 100644
--- a/io_uring/futex.c
+++ b/io_uring/futex.c
@@ -36,7 +36,7 @@ struct io_futex_data {
 bool io_futex_cache_init(struct io_ring_ctx *ctx)
 {
 	return io_alloc_cache_init(&ctx->futex_cache, IO_FUTEX_ALLOC_CACHE_MAX,
-				sizeof(struct io_futex_data));
+				sizeof(struct io_futex_data), 0);
 }
 
 void io_futex_cache_free(struct io_ring_ctx *ctx)
@@ -320,7 +320,7 @@ int io_futex_wait(struct io_kiocb *req, unsigned int issue_flags)
 	}
 
 	io_ring_submit_lock(ctx, issue_flags);
-	ifd = io_cache_alloc(&ctx->futex_cache, GFP_NOWAIT, NULL);
+	ifd = io_cache_alloc(&ctx->futex_cache, GFP_NOWAIT);
 	if (!ifd) {
 		ret = -ENOMEM;
 		goto done_unlock;
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 7bfbc7c22367..7d38bdccbad8 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -315,16 +315,19 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 	INIT_LIST_HEAD(&ctx->cq_overflow_list);
 	INIT_LIST_HEAD(&ctx->io_buffers_cache);
 	ret = io_alloc_cache_init(&ctx->apoll_cache, IO_POLL_ALLOC_CACHE_MAX,
-			    sizeof(struct async_poll));
+			    sizeof(struct async_poll), 0);
 	ret |= io_alloc_cache_init(&ctx->netmsg_cache, IO_ALLOC_CACHE_MAX,
-			    sizeof(struct io_async_msghdr));
+			    sizeof(struct io_async_msghdr),
+			    offsetof(struct io_async_msghdr, init));
 	ret |= io_alloc_cache_init(&ctx->rw_cache, IO_ALLOC_CACHE_MAX,
-			    sizeof(struct io_async_rw));
+			    sizeof(struct io_async_rw),
+			    offsetof(struct io_async_rw, init));
 	ret |= io_alloc_cache_init(&ctx->uring_cache, IO_ALLOC_CACHE_MAX,
-			    sizeof(struct io_uring_cmd_data));
+			    sizeof(struct io_uring_cmd_data),
+			    offsetof(struct io_uring_cmd_data, init));
 	spin_lock_init(&ctx->msg_lock);
 	ret |= io_alloc_cache_init(&ctx->msg_cache, IO_ALLOC_CACHE_MAX,
-			    sizeof(struct io_kiocb));
+			    sizeof(struct io_kiocb), 0);
 	ret |= io_futex_cache_init(ctx);
 	if (ret)
 		goto free_ref;
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index f65e3f3ede51..67adbb3c1bf5 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -226,10 +226,9 @@ static inline void io_req_set_res(struct io_kiocb *req, s32 res, u32 cflags)
 }
 
 static inline void *io_uring_alloc_async_data(struct io_alloc_cache *cache,
-					      struct io_kiocb *req,
-					      void (*init_once)(void *obj))
+					      struct io_kiocb *req)
 {
-	req->async_data = io_cache_alloc(cache, GFP_KERNEL, init_once);
+	req->async_data = io_cache_alloc(cache, GFP_KERNEL);
 	if (req->async_data)
 		req->flags |= REQ_F_ASYNC_DATA;
 	return req->async_data;
diff --git a/io_uring/net.c b/io_uring/net.c
index 85f55fbc25c9..35bec4680fcc 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -155,21 +155,12 @@ static void io_netmsg_recycle(struct io_kiocb *req, unsigned int issue_flags)
 	}
 }
 
-static void io_msg_async_data_init(void *obj)
-{
-	struct io_async_msghdr *hdr = (struct io_async_msghdr *)obj;
-
-	hdr->free_iov = NULL;
-	hdr->free_iov_nr = 0;
-}
-
 static struct io_async_msghdr *io_msg_alloc_async(struct io_kiocb *req)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 	struct io_async_msghdr *hdr;
 
-	hdr = io_uring_alloc_async_data(&ctx->netmsg_cache, req,
-					io_msg_async_data_init);
+	hdr = io_uring_alloc_async_data(&ctx->netmsg_cache, req);
 	if (!hdr)
 		return NULL;
 
diff --git a/io_uring/net.h b/io_uring/net.h
index 52bfee05f06a..22a1a34bca09 100644
--- a/io_uring/net.h
+++ b/io_uring/net.h
@@ -5,16 +5,19 @@
 
 struct io_async_msghdr {
 #if defined(CONFIG_NET)
-	struct iovec			fast_iov;
-	/* points to an allocated iov, if NULL we use fast_iov instead */
 	struct iovec			*free_iov;
+	/* points to an allocated iov, if NULL we use fast_iov instead */
 	int				free_iov_nr;
+	int				init[0];
 	int				namelen;
+	struct iovec			fast_iov;
 	__kernel_size_t			controllen;
 	__kernel_size_t			payloadlen;
 	struct sockaddr __user		*uaddr;
 	struct msghdr			msg;
 	struct sockaddr_storage		addr;
+#else
+	int				init[0];
 #endif
 };
 
diff --git a/io_uring/poll.c b/io_uring/poll.c
index cc01c40b43d3..356474c66f32 100644
--- a/io_uring/poll.c
+++ b/io_uring/poll.c
@@ -650,7 +650,7 @@ static struct async_poll *io_req_alloc_apoll(struct io_kiocb *req,
 		kfree(apoll->double_poll);
 	} else {
 		if (!(issue_flags & IO_URING_F_UNLOCKED))
-			apoll = io_cache_alloc(&ctx->apoll_cache, GFP_ATOMIC, NULL);
+			apoll = io_cache_alloc(&ctx->apoll_cache, GFP_ATOMIC);
 		else
 			apoll = kmalloc(sizeof(*apoll), GFP_ATOMIC);
 		if (!apoll)
diff --git a/io_uring/rw.c b/io_uring/rw.c
index a9a2733be842..0a0ad8a2bc0d 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -208,20 +208,12 @@ static void io_req_rw_cleanup(struct io_kiocb *req, unsigned int issue_flags)
 	}
 }
 
-static void io_rw_async_data_init(void *obj)
-{
-	struct io_async_rw *rw = (struct io_async_rw *)obj;
-
-	rw->free_iovec = NULL;
-	rw->bytes_done = 0;
-}
-
 static int io_rw_alloc_async(struct io_kiocb *req)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 	struct io_async_rw *rw;
 
-	rw = io_uring_alloc_async_data(&ctx->rw_cache, req, io_rw_async_data_init);
+	rw = io_uring_alloc_async_data(&ctx->rw_cache, req);
 	if (!rw)
 		return -ENOMEM;
 	if (rw->free_iovec) {
diff --git a/io_uring/rw.h b/io_uring/rw.h
index 2d7656bd268d..ab9e4df2504a 100644
--- a/io_uring/rw.h
+++ b/io_uring/rw.h
@@ -8,11 +8,14 @@ struct io_meta_state {
 };
 
 struct io_async_rw {
+	/* these must be cleared on initial alloc */
 	size_t				bytes_done;
+	struct iovec			*free_iovec;
+	int				init[0];
+	/* rest persists over caching */
 	struct iov_iter			iter;
 	struct iov_iter_state		iter_state;
 	struct iovec			fast_iov;
-	struct iovec			*free_iovec;
 	int				free_iov_nr;
 	/* wpq is for buffered io, while meta fields are used with direct io */
 	union {
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index 6a63ec4b5445..0da7dc20eca6 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -168,21 +168,13 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, u64 res2,
 }
 EXPORT_SYMBOL_GPL(io_uring_cmd_done);
 
-static void io_uring_cmd_init_once(void *obj)
-{
-	struct io_uring_cmd_data *data = obj;
-
-	data->op_data = NULL;
-}	
-
 static int io_uring_cmd_prep_setup(struct io_kiocb *req,
 				   const struct io_uring_sqe *sqe)
 {
 	struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd);
 	struct io_uring_cmd_data *cache;
 
-	cache = io_uring_alloc_async_data(&req->ctx->uring_cache, req,
-			io_uring_cmd_init_once);
+	cache = io_uring_alloc_async_data(&req->ctx->uring_cache, req);
 	if (!cache)
 		return -ENOMEM;
 
-- 
2.47.2

Re: [PATCH 2/2] io_uring: get rid of alloc cache init_once handling

[Pavel Begunkov]

On 1/23/25 14:21, Jens Axboe wrote:
> init_once is called when an object doesn't come from the cache, and
> hence needs initial clearing of certain members. While the whole
> struct could get cleared by memset() in that case, a few of the cache
> members are large enough that this may cause unnecessary overhead if
> the caches used aren't large enough to satisfy the workload. For those
> cases, some churn of kmalloc+kfree is to be expected.
> 
> Ensure that the 3 users that need clearing put the members they need
> cleared at the start of the struct, and place an empty placeholder
> 'init' member so that the cache initialization knows how much to
> clear.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>   include/linux/io_uring/cmd.h   |  3 ++-
>   include/linux/io_uring_types.h |  3 ++-
>   io_uring/alloc_cache.h         | 30 +++++++++++++++++++++---------
>   io_uring/futex.c               |  4 ++--
>   io_uring/io_uring.c            | 13 ++++++++-----
>   io_uring/io_uring.h            |  5 ++---
>   io_uring/net.c                 | 11 +----------
>   io_uring/net.h                 |  7 +++++--
>   io_uring/poll.c                |  2 +-
>   io_uring/rw.c                  | 10 +---------
>   io_uring/rw.h                  |  5 ++++-
>   io_uring/uring_cmd.c           | 10 +---------
>   12 files changed, 50 insertions(+), 53 deletions(-)
> 
> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
> index a3ce553413de..8d7746d9fd23 100644
> --- a/include/linux/io_uring/cmd.h
> +++ b/include/linux/io_uring/cmd.h
> @@ -19,8 +19,9 @@ struct io_uring_cmd {
>   };
>   
>   struct io_uring_cmd_data {
> -	struct io_uring_sqe	sqes[2];
>   	void			*op_data;
> +	int			init[0];

What do you think about using struct_group instead?

-- 
Pavel Begunkov

Re: [PATCH 1/2] io_uring/uring_cmd: cleanup struct io_uring_cmd_data layout

[Pavel Begunkov]

On 1/23/25 14:21, Jens Axboe wrote:
> A few spots in uring_cmd assume that the SQEs copied are always at the
> start of the structure, and hence mix req->async_data and the struct
> itself.
> 
> Clean that up and use the proper indices.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>   io_uring/uring_cmd.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> index 3993c9339ac7..6a63ec4b5445 100644
> --- a/io_uring/uring_cmd.c
> +++ b/io_uring/uring_cmd.c
> @@ -192,8 +192,8 @@ static int io_uring_cmd_prep_setup(struct io_kiocb *req,
>   		return 0;
>   	}
>   
> -	memcpy(req->async_data, sqe, uring_sqe_size(req->ctx));
> -	ioucmd->sqe = req->async_data;
> +	memcpy(cache->sqes, sqe, uring_sqe_size(req->ctx));
> +	ioucmd->sqe = cache->sqes;
>   	return 0;
>   }
>   
> @@ -260,7 +260,7 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>   		struct io_uring_cmd_data *cache = req->async_data;
>   
>   		if (ioucmd->sqe != (void *) cache)
> -			memcpy(cache, ioucmd->sqe, uring_sqe_size(req->ctx));
> +			memcpy(cache->sqes, ioucmd->sqe, uring_sqe_size(req->ctx));

3347fa658a1b ("io_uring/cmd: add per-op data to struct io_uring_cmd_data")

IIUC the patch above is queued for 6.14, and with that this patch
looks like a fix? At least it feels pretty dangerous without.

-- 
Pavel Begunkov

Re: [PATCH 2/2] io_uring: get rid of alloc cache init_once handling

[Pavel Begunkov]

On 1/23/25 14:27, Pavel Begunkov wrote:
> On 1/23/25 14:21, Jens Axboe wrote:
>> init_once is called when an object doesn't come from the cache, and
>> hence needs initial clearing of certain members. While the whole
>> struct could get cleared by memset() in that case, a few of the cache
>> members are large enough that this may cause unnecessary overhead if
>> the caches used aren't large enough to satisfy the workload. For those
>> cases, some churn of kmalloc+kfree is to be expected.
>>
>> Ensure that the 3 users that need clearing put the members they need
>> cleared at the start of the struct, and place an empty placeholder
>> 'init' member so that the cache initialization knows how much to
>> clear.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>   include/linux/io_uring/cmd.h   |  3 ++-
>>   include/linux/io_uring_types.h |  3 ++-
>>   io_uring/alloc_cache.h         | 30 +++++++++++++++++++++---------
>>   io_uring/futex.c               |  4 ++--
>>   io_uring/io_uring.c            | 13 ++++++++-----
>>   io_uring/io_uring.h            |  5 ++---
>>   io_uring/net.c                 | 11 +----------
>>   io_uring/net.h                 |  7 +++++--
>>   io_uring/poll.c                |  2 +-
>>   io_uring/rw.c                  | 10 +---------
>>   io_uring/rw.h                  |  5 ++++-
>>   io_uring/uring_cmd.c           | 10 +---------
>>   12 files changed, 50 insertions(+), 53 deletions(-)
>>
>> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
>> index a3ce553413de..8d7746d9fd23 100644
>> --- a/include/linux/io_uring/cmd.h
>> +++ b/include/linux/io_uring/cmd.h
>> @@ -19,8 +19,9 @@ struct io_uring_cmd {
>>   };
>>   struct io_uring_cmd_data {
>> -    struct io_uring_sqe    sqes[2];
>>       void            *op_data;
>> +    int            init[0];
> 
> What do you think about using struct_group instead?

And why do we care not clearing it all on initial alloc? If that's
because of kasan, we can disable it until ("kasan, mempool: don't
store free stacktrace in io_alloc_cache objects") lands.

-- 
Pavel Begunkov

Re: [PATCH 1/2] io_uring/uring_cmd: cleanup struct io_uring_cmd_data layout

[Jens Axboe]

On 1/23/25 7:38 AM, Pavel Begunkov wrote:
> On 1/23/25 14:21, Jens Axboe wrote:
>> A few spots in uring_cmd assume that the SQEs copied are always at the
>> start of the structure, and hence mix req->async_data and the struct
>> itself.
>>
>> Clean that up and use the proper indices.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>   io_uring/uring_cmd.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>> index 3993c9339ac7..6a63ec4b5445 100644
>> --- a/io_uring/uring_cmd.c
>> +++ b/io_uring/uring_cmd.c
>> @@ -192,8 +192,8 @@ static int io_uring_cmd_prep_setup(struct io_kiocb *req,
>>           return 0;
>>       }
>>   -    memcpy(req->async_data, sqe, uring_sqe_size(req->ctx));
>> -    ioucmd->sqe = req->async_data;
>> +    memcpy(cache->sqes, sqe, uring_sqe_size(req->ctx));
>> +    ioucmd->sqe = cache->sqes;
>>       return 0;
>>   }
>>   @@ -260,7 +260,7 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>           struct io_uring_cmd_data *cache = req->async_data;
>>             if (ioucmd->sqe != (void *) cache)
>> -            memcpy(cache, ioucmd->sqe, uring_sqe_size(req->ctx));
>> +            memcpy(cache->sqes, ioucmd->sqe, uring_sqe_size(req->ctx));
> 
> 3347fa658a1b ("io_uring/cmd: add per-op data to struct io_uring_cmd_data")
> 
> IIUC the patch above is queued for 6.14, and with that this patch
> looks like a fix? At least it feels pretty dangerous without.

It's not a fix, the sqes are first in the struct even with that patch.
So I'd consider it a cleanup. In any case, targeting 6.14 for these
alloc cache cleanups as it got introduced there as well.

-- 
Jens Axboe

Re: [PATCH 2/2] io_uring: get rid of alloc cache init_once handling

[Jens Axboe]

On 1/23/25 7:27 AM, Pavel Begunkov wrote:
> On 1/23/25 14:21, Jens Axboe wrote:
>> init_once is called when an object doesn't come from the cache, and
>> hence needs initial clearing of certain members. While the whole
>> struct could get cleared by memset() in that case, a few of the cache
>> members are large enough that this may cause unnecessary overhead if
>> the caches used aren't large enough to satisfy the workload. For those
>> cases, some churn of kmalloc+kfree is to be expected.
>>
>> Ensure that the 3 users that need clearing put the members they need
>> cleared at the start of the struct, and place an empty placeholder
>> 'init' member so that the cache initialization knows how much to
>> clear.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>   include/linux/io_uring/cmd.h   |  3 ++-
>>   include/linux/io_uring_types.h |  3 ++-
>>   io_uring/alloc_cache.h         | 30 +++++++++++++++++++++---------
>>   io_uring/futex.c               |  4 ++--
>>   io_uring/io_uring.c            | 13 ++++++++-----
>>   io_uring/io_uring.h            |  5 ++---
>>   io_uring/net.c                 | 11 +----------
>>   io_uring/net.h                 |  7 +++++--
>>   io_uring/poll.c                |  2 +-
>>   io_uring/rw.c                  | 10 +---------
>>   io_uring/rw.h                  |  5 ++++-
>>   io_uring/uring_cmd.c           | 10 +---------
>>   12 files changed, 50 insertions(+), 53 deletions(-)
>>
>> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
>> index a3ce553413de..8d7746d9fd23 100644
>> --- a/include/linux/io_uring/cmd.h
>> +++ b/include/linux/io_uring/cmd.h
>> @@ -19,8 +19,9 @@ struct io_uring_cmd {
>>   };
>>     struct io_uring_cmd_data {
>> -    struct io_uring_sqe    sqes[2];
>>       void            *op_data;
>> +    int            init[0];
> 
> What do you think about using struct_group instead?

That's a good idea, better than the magic empty variable. I'll make that
change for v2, thanks!

-- 
Jens Axboe

Re: [PATCH 2/2] io_uring: get rid of alloc cache init_once handling

[Jens Axboe]

On 1/23/25 7:47 AM, Pavel Begunkov wrote:
> On 1/23/25 14:27, Pavel Begunkov wrote:
>> On 1/23/25 14:21, Jens Axboe wrote:
>>> init_once is called when an object doesn't come from the cache, and
>>> hence needs initial clearing of certain members. While the whole
>>> struct could get cleared by memset() in that case, a few of the cache
>>> members are large enough that this may cause unnecessary overhead if
>>> the caches used aren't large enough to satisfy the workload. For those
>>> cases, some churn of kmalloc+kfree is to be expected.
>>>
>>> Ensure that the 3 users that need clearing put the members they need
>>> cleared at the start of the struct, and place an empty placeholder
>>> 'init' member so that the cache initialization knows how much to
>>> clear.
>>>
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>> ---
>>>   include/linux/io_uring/cmd.h   |  3 ++-
>>>   include/linux/io_uring_types.h |  3 ++-
>>>   io_uring/alloc_cache.h         | 30 +++++++++++++++++++++---------
>>>   io_uring/futex.c               |  4 ++--
>>>   io_uring/io_uring.c            | 13 ++++++++-----
>>>   io_uring/io_uring.h            |  5 ++---
>>>   io_uring/net.c                 | 11 +----------
>>>   io_uring/net.h                 |  7 +++++--
>>>   io_uring/poll.c                |  2 +-
>>>   io_uring/rw.c                  | 10 +---------
>>>   io_uring/rw.h                  |  5 ++++-
>>>   io_uring/uring_cmd.c           | 10 +---------
>>>   12 files changed, 50 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
>>> index a3ce553413de..8d7746d9fd23 100644
>>> --- a/include/linux/io_uring/cmd.h
>>> +++ b/include/linux/io_uring/cmd.h
>>> @@ -19,8 +19,9 @@ struct io_uring_cmd {
>>>   };
>>>   struct io_uring_cmd_data {
>>> -    struct io_uring_sqe    sqes[2];
>>>       void            *op_data;
>>> +    int            init[0];
>>
>> What do you think about using struct_group instead?
> 
> And why do we care not clearing it all on initial alloc? If that's
> because of kasan, we can disable it until ("kasan, mempool: don't
> store free stacktrace in io_alloc_cache objects") lands.

Not sure I follow - on initial alloc they do need clearing, that's when
they need clearing. If they are coming from the cache, the state should
be consistent.

-- 
Jens Axboe

Re: [PATCH 1/2] io_uring/uring_cmd: cleanup struct io_uring_cmd_data layout

[Pavel Begunkov]

On 1/23/25 14:54, Jens Axboe wrote:
> On 1/23/25 7:38 AM, Pavel Begunkov wrote:
>> On 1/23/25 14:21, Jens Axboe wrote:
>>> A few spots in uring_cmd assume that the SQEs copied are always at the
>>> start of the structure, and hence mix req->async_data and the struct
>>> itself.
>>>
>>> Clean that up and use the proper indices.
>>>
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>> ---
>>>    io_uring/uring_cmd.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>>> index 3993c9339ac7..6a63ec4b5445 100644
>>> --- a/io_uring/uring_cmd.c
>>> +++ b/io_uring/uring_cmd.c
>>> @@ -192,8 +192,8 @@ static int io_uring_cmd_prep_setup(struct io_kiocb *req,
>>>            return 0;
>>>        }
>>>    -    memcpy(req->async_data, sqe, uring_sqe_size(req->ctx));
>>> -    ioucmd->sqe = req->async_data;
>>> +    memcpy(cache->sqes, sqe, uring_sqe_size(req->ctx));
>>> +    ioucmd->sqe = cache->sqes;
>>>        return 0;
>>>    }
>>>    @@ -260,7 +260,7 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>>            struct io_uring_cmd_data *cache = req->async_data;
>>>              if (ioucmd->sqe != (void *) cache)
>>> -            memcpy(cache, ioucmd->sqe, uring_sqe_size(req->ctx));
>>> +            memcpy(cache->sqes, ioucmd->sqe, uring_sqe_size(req->ctx));
>>
>> 3347fa658a1b ("io_uring/cmd: add per-op data to struct io_uring_cmd_data")
>>
>> IIUC the patch above is queued for 6.14, and with that this patch
>> looks like a fix? At least it feels pretty dangerous without.
> 
> It's not a fix, the sqes are first in the struct even with that patch.

Ah yes

> So I'd consider it a cleanup. In any case, targeting 6.14 for these
> alloc cache cleanups as it got introduced there as well.

That's good, makes it not that brittle

-- 
Pavel Begunkov

Re: [PATCH 1/2] io_uring/uring_cmd: cleanup struct io_uring_cmd_data layout

[Jens Axboe]

On 1/23/25 7:57 AM, Pavel Begunkov wrote:
> On 1/23/25 14:54, Jens Axboe wrote:
>> On 1/23/25 7:38 AM, Pavel Begunkov wrote:
>>> On 1/23/25 14:21, Jens Axboe wrote:
>>>> A few spots in uring_cmd assume that the SQEs copied are always at the
>>>> start of the structure, and hence mix req->async_data and the struct
>>>> itself.
>>>>
>>>> Clean that up and use the proper indices.
>>>>
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>> ---
>>>>    io_uring/uring_cmd.c | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>>>> index 3993c9339ac7..6a63ec4b5445 100644
>>>> --- a/io_uring/uring_cmd.c
>>>> +++ b/io_uring/uring_cmd.c
>>>> @@ -192,8 +192,8 @@ static int io_uring_cmd_prep_setup(struct io_kiocb *req,
>>>>            return 0;
>>>>        }
>>>>    -    memcpy(req->async_data, sqe, uring_sqe_size(req->ctx));
>>>> -    ioucmd->sqe = req->async_data;
>>>> +    memcpy(cache->sqes, sqe, uring_sqe_size(req->ctx));
>>>> +    ioucmd->sqe = cache->sqes;
>>>>        return 0;
>>>>    }
>>>>    @@ -260,7 +260,7 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>>>            struct io_uring_cmd_data *cache = req->async_data;
>>>>              if (ioucmd->sqe != (void *) cache)
>>>> -            memcpy(cache, ioucmd->sqe, uring_sqe_size(req->ctx));
>>>> +            memcpy(cache->sqes, ioucmd->sqe, uring_sqe_size(req->ctx));
>>>
>>> 3347fa658a1b ("io_uring/cmd: add per-op data to struct io_uring_cmd_data")
>>>
>>> IIUC the patch above is queued for 6.14, and with that this patch
>>> looks like a fix? At least it feels pretty dangerous without.
>>
>> It's not a fix, the sqes are first in the struct even with that patch.
> 
> Ah yes
> 
>> So I'd consider it a cleanup. In any case, targeting 6.14 for these
>> alloc cache cleanups as it got introduced there as well.
> 
> That's good, makes it not that brittle

Yep it was too easy to miss, don't like them being aliased like that
even if the usage was currently fine.

-- 
Jens Axboe

Re: [PATCH 2/2] io_uring: get rid of alloc cache init_once handling

[Pavel Begunkov]

On 1/23/25 14:55, Jens Axboe wrote:
> On 1/23/25 7:47 AM, Pavel Begunkov wrote:
>> On 1/23/25 14:27, Pavel Begunkov wrote:
>>> On 1/23/25 14:21, Jens Axboe wrote:
>>>> init_once is called when an object doesn't come from the cache, and
>>>> hence needs initial clearing of certain members. While the whole
>>>> struct could get cleared by memset() in that case, a few of the cache
>>>> members are large enough that this may cause unnecessary overhead if
>>>> the caches used aren't large enough to satisfy the workload. For those
>>>> cases, some churn of kmalloc+kfree is to be expected.
>>>>
>>>> Ensure that the 3 users that need clearing put the members they need
>>>> cleared at the start of the struct, and place an empty placeholder
>>>> 'init' member so that the cache initialization knows how much to
>>>> clear.
>>>>
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>> ---
>>>>    include/linux/io_uring/cmd.h   |  3 ++-
>>>>    include/linux/io_uring_types.h |  3 ++-
>>>>    io_uring/alloc_cache.h         | 30 +++++++++++++++++++++---------
>>>>    io_uring/futex.c               |  4 ++--
>>>>    io_uring/io_uring.c            | 13 ++++++++-----
>>>>    io_uring/io_uring.h            |  5 ++---
>>>>    io_uring/net.c                 | 11 +----------
>>>>    io_uring/net.h                 |  7 +++++--
>>>>    io_uring/poll.c                |  2 +-
>>>>    io_uring/rw.c                  | 10 +---------
>>>>    io_uring/rw.h                  |  5 ++++-
>>>>    io_uring/uring_cmd.c           | 10 +---------
>>>>    12 files changed, 50 insertions(+), 53 deletions(-)
>>>>
>>>> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
>>>> index a3ce553413de..8d7746d9fd23 100644
>>>> --- a/include/linux/io_uring/cmd.h
>>>> +++ b/include/linux/io_uring/cmd.h
>>>> @@ -19,8 +19,9 @@ struct io_uring_cmd {
>>>>    };
>>>>    struct io_uring_cmd_data {
>>>> -    struct io_uring_sqe    sqes[2];
>>>>        void            *op_data;
>>>> +    int            init[0];
>>>
>>> What do you think about using struct_group instead?
>>
>> And why do we care not clearing it all on initial alloc? If that's
>> because of kasan, we can disable it until ("kasan, mempool: don't
>> store free stacktrace in io_alloc_cache objects") lands.
> 
> Not sure I follow - on initial alloc they do need clearing, that's when
> they need clearing. If they are coming from the cache, the state should
> be consistent.

If we forget about kasan, ->init_clear is only really used right
after allocation().

+	obj = kmalloc(cache->elem_size, gfp);
+	if (obj && cache->init_clear)
+		memset(obj, 0, cache->init_clear);

Why not kzalloc() it?

-- 
Pavel Begunkov

Re: [PATCH 2/2] io_uring: get rid of alloc cache init_once handling

[Jens Axboe]

On 1/23/25 8:05 AM, Pavel Begunkov wrote:
> On 1/23/25 14:55, Jens Axboe wrote:
>> On 1/23/25 7:47 AM, Pavel Begunkov wrote:
>>> On 1/23/25 14:27, Pavel Begunkov wrote:
>>>> On 1/23/25 14:21, Jens Axboe wrote:
>>>>> init_once is called when an object doesn't come from the cache, and
>>>>> hence needs initial clearing of certain members. While the whole
>>>>> struct could get cleared by memset() in that case, a few of the cache
>>>>> members are large enough that this may cause unnecessary overhead if
>>>>> the caches used aren't large enough to satisfy the workload. For those
>>>>> cases, some churn of kmalloc+kfree is to be expected.
>>>>>
>>>>> Ensure that the 3 users that need clearing put the members they need
>>>>> cleared at the start of the struct, and place an empty placeholder
>>>>> 'init' member so that the cache initialization knows how much to
>>>>> clear.
>>>>>
>>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>> ---
>>>>>    include/linux/io_uring/cmd.h   |  3 ++-
>>>>>    include/linux/io_uring_types.h |  3 ++-
>>>>>    io_uring/alloc_cache.h         | 30 +++++++++++++++++++++---------
>>>>>    io_uring/futex.c               |  4 ++--
>>>>>    io_uring/io_uring.c            | 13 ++++++++-----
>>>>>    io_uring/io_uring.h            |  5 ++---
>>>>>    io_uring/net.c                 | 11 +----------
>>>>>    io_uring/net.h                 |  7 +++++--
>>>>>    io_uring/poll.c                |  2 +-
>>>>>    io_uring/rw.c                  | 10 +---------
>>>>>    io_uring/rw.h                  |  5 ++++-
>>>>>    io_uring/uring_cmd.c           | 10 +---------
>>>>>    12 files changed, 50 insertions(+), 53 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
>>>>> index a3ce553413de..8d7746d9fd23 100644
>>>>> --- a/include/linux/io_uring/cmd.h
>>>>> +++ b/include/linux/io_uring/cmd.h
>>>>> @@ -19,8 +19,9 @@ struct io_uring_cmd {
>>>>>    };
>>>>>    struct io_uring_cmd_data {
>>>>> -    struct io_uring_sqe    sqes[2];
>>>>>        void            *op_data;
>>>>> +    int            init[0];
>>>>
>>>> What do you think about using struct_group instead?
>>>
>>> And why do we care not clearing it all on initial alloc? If that's
>>> because of kasan, we can disable it until ("kasan, mempool: don't
>>> store free stacktrace in io_alloc_cache objects") lands.
>>
>> Not sure I follow - on initial alloc they do need clearing, that's when
>> they need clearing. If they are coming from the cache, the state should
>> be consistent.
> 
> If we forget about kasan, ->init_clear is only really used right
> after allocation().
> 
> +    obj = kmalloc(cache->elem_size, gfp);
> +    if (obj && cache->init_clear)
> +        memset(obj, 0, cache->init_clear);
> 
> Why not kzalloc() it?

We obviously could, but rw/net is 2-300b in size, and most of it we
don't need to clear. Yes this is only for the "slower" path of hitting
alloc rather than the cache, but it's not a given at all that any
workload will fit within the cache, unfortunately. That's quite a lot of
memset for those cases.

But maybe I'm overdoing it and we just kzalloc() it. It'd obviously be a
lot simpler.

-- 
Jens Axboe