On 3/21/24 9:59 AM, Gabriel Krisman Bertazi wrote: > Jens Axboe <axboe@xxxxxxxxx> 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. >> -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. -- Jens Axboe