Re: [PATCH 17/17] io_uring/alloc_cache: switch to array based caching

[Gabriel Krisman Bertazi <krisman@suse.de>]

Jens Axboe <axboe@kernel.dk> writes:

> On 3/21/24 9:59 AM, Gabriel Krisman Bertazi wrote:
>> Jens Axboe <axboe@kernel.dk> writes:
>> 
>>> Currently lists are being used to manage this, but lists isn't a very
>>> good choice for as extracting the current entry necessitates touching
>>> the next entry as well, to update the list head.
>>>
>>> Outside of that detail, games are also played with KASAN as the list
>>> is inside the cached entry itself.
>>>
>>> Finally, all users of this need a struct io_cache_entry embedded in
>>> their struct, which is union'ized with something else in there that
>>> isn't used across the free -> realloc cycle.
>>>
>>> Get rid of all of that, and simply have it be an array. This will not
>>> change the memory used, as we're just trading an 8-byte member entry
>>> for the per-elem array size.
>>>
>>> This reduces the overhead of the recycled allocations, and it reduces
>>> the code we have to support recycling.
>> 
>> Hi Jens,
>> 
>> I tried applying the entire to your for-6.10/io_uring branch to test it
>> and only this last patch failed to apply. The tip of the branch I have
>> is 22261e73e8d2 ("io_uring/alloc_cache: shrink default max entries from
>> 512 to 128").
>
> Yeah it has some dependencies that need unraveling. The easiest is if
> you just pull:
>
> git://git.kernel.dk/linux io_uring-recvsend-bundle
>
> into current -git master, and then just test that. That gets you pretty
> much everything that's being tested and played with.
>
> Top of tree is d5653d2fcf1383c0fbe8b64545664aea36c7aca2 right now.

thanks, I'll test with that.

>
>>> -static inline struct io_cache_entry *io_alloc_cache_get(struct io_alloc_cache *cache)
>>> +static inline void *io_alloc_cache_get(struct io_alloc_cache *cache)
>>>  {
>>> -	if (cache->list.next) {
>>> -		struct io_cache_entry *entry;
>>> +	if (cache->nr_cached) {
>>> +		void *entry = cache->entries[--cache->nr_cached];
>>>  
>>> -		entry = container_of(cache->list.next, struct io_cache_entry, node);
>>>  		kasan_mempool_unpoison_object(entry, cache->elem_size);
>>> -		cache->list.next = cache->list.next->next;
>>> -		cache->nr_cached--;
>>>  		return entry;
>>>  	}
>>>  
>>>  	return NULL;
>>>  }
>>>  
>>> -static inline void io_alloc_cache_init(struct io_alloc_cache *cache,
>>> -				       unsigned max_nr, size_t size)
>>> +static inline int io_alloc_cache_init(struct io_alloc_cache *cache,
>>> +				      unsigned max_nr, size_t size)
>>>  {
>>> -	cache->list.next = NULL;
>>> +	cache->entries = kvmalloc_array(max_nr, sizeof(void *), GFP_KERNEL);
>>> +	if (!cache->entries)
>>> +		return -ENOMEM;
>>>  	cache->nr_cached = 0;
>>>  	cache->max_cached = max_nr;
>>>  	cache->elem_size = size;
>>> +	return 0;
>>>  }
>>>  
>>>  static inline void io_alloc_cache_free(struct io_alloc_cache *cache,
>>> -					void (*free)(struct io_cache_entry *))
>>> +				       void (*free)(const void *))
>> 
>> Minor, but since free is supposed to free the entry, const doesn't
>> make sense here.  Also, you actually just cast it away immediately in
>> every usage.
>
> It's because then I can use kfree() directly for most cases, only two of
> them have special freeing functions. And kfree takes a const void *. I
> should add a comment about that.

TIL. For the record, I was very puzzled on why kfree receives a const
pointer just to cast it away immediately too. Then I found Linus
discussing it at https://yarchive.net/comp/const.html

Anyway, in this case, we are actually modifying it in io_rw_cache_free,
and we don't need to explicitly cast from non-const to const , so I still
think you can avoid the comment and drop the const.  But that is just
a nitpick that i won't insist.

Thanks!

-- 
Gabriel Krisman Bertazi