Re: [RFC PATCH 1/3] io_uring/rsrc: allocate io_rsrc_node from a dedicated kmem_cache

[Gabriel Krisman Bertazi]

Uzair Beg <uzairbeg11@xxxxxxxxx> writes:

> io_rsrc_node allocations come from the generic kmalloc-32 bucket via
> io_cache_alloc_new(). On a first fill of a sparse fixed file table the
> per-ring node cache is empty by construction, since io_reset_rsrc_node()
> returns early on a NULL slot and nothing is freed back, so every install
> takes an allocator round trip.
>
> Add an optional kmem_cache to io_alloc_cache and use it for the node
> cache. The slab pointer defaults to NULL, so other io_alloc_cache users
> are unchanged and imu_cache keeps using kmalloc, its element size being
> variable. All three free paths honour the slab.
>
> On its own this is neutral on the reported workload. It exists so that
> the following patches can use kmem_cache_alloc_bulk(), which has no
> equivalent for plain kmalloc.
>
> Reported-by: Chengfeng Lin <lin2530632123@xxxxxxxxx>
> Closes: https://lore.kernel.org/io-uring/CANGjgdmt0FQ=offsdfn+wEaDxbOFoAa6bi92X_vEo4S6aCZ56A@xxxxxxxxxxxxxx/
> Tested-by: Chengfeng Lin <lin2530632123@xxxxxxxxx>
> Signed-off-by: Uzair Beg <uzairbeg11@xxxxxxxxx>
> ---
>  include/linux/io_uring_types.h |  1 +
>  io_uring/alloc_cache.c         | 14 +++++++++++---
>  io_uring/alloc_cache.h         |  8 ++++++--
>  io_uring/io_uring.c            |  5 +++++
>  io_uring/io_uring.h            |  1 +
>  io_uring/rsrc.c                |  2 ++
>  6 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
> index c2ea6280901..e8d5a585a60 100644
> --- a/include/linux/io_uring_types.h
> +++ b/include/linux/io_uring_types.h
> @@ -253,6 +253,7 @@ struct io_alloc_cache {
>  	unsigned int		max_cached;
>  	unsigned int		elem_size;
>  	unsigned int		init_clear;
> +	struct kmem_cache	*slab;
>  };

Not opposed to having a slab, but I actually think it is
unnecessary.  see patch 2.  If we do that, we actually should delete the
async_size parameter in the opdef, since it is now redundant.  But
see comments in patch 2.

>  
>  struct io_ring_ctx {
> diff --git a/io_uring/alloc_cache.c b/io_uring/alloc_cache.c
> index 58423888b73..a44b82a80f1 100644
> --- a/io_uring/alloc_cache.c
> +++ b/io_uring/alloc_cache.c
> @@ -10,8 +10,12 @@ void io_alloc_cache_free(struct io_alloc_cache *cache,
>  	if (!cache->entries)
>  		return;
>  
> -	while ((entry = io_alloc_cache_get(cache)) != NULL)
> -		free(entry);
> +	while ((entry = io_alloc_cache_get(cache)) != NULL) {
> +		if (cache->slab)
> +			kmem_cache_free(cache->slab, entry);
> +		else
> +			free(entry);
> +	}

FWIW, this is exactly why we have the (*free) callback, you should have a function
for your specific type used as a callback that does the kmem_cache_free
so you don't need the if/else here.

But for a simple kmem_cache_free, kfree works just fine.

-- 
Gabriel Krisman Bertazi