On 6/4/24 7:53 PM, Yosry Ahmed wrote: > Zswap creates multiple zpools to improve concurrency. Each zsmalloc > zpool creates its own 'zs_handle' and 'zspage' slab caches. Currently we > end up with 32 slab caches of each type. > > Since each slab cache holds some free objects, we end up with a lot of > free objects distributed among the separate zpool caches. Slab caches > are designed to handle concurrent allocations by using percpu > structures, so having a single instance of each cache should be enough, > and avoids wasting more memory than needed due to fragmentation. > > Additionally, having more slab caches than needed unnecessarily slows > down code paths that iterate slab_caches. > > In the results reported by Eric in [1], the amount of unused slab memory > in these caches goes down from 242808 bytes to 29216 bytes (-88%). This > is calculated by (num_objs - active_objs) * objsize for each 'zs_handle' > and 'zspage' cache. Although this patch did not help with the allocation > failure reported by Eric with zswap + zsmalloc, I think it is still > worth merging on its own. > > [1]https://lore.kernel.org/lkml/20240604134458.3ae4396a@yea/ > > Signed-off-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx> As mentioned in the thread, CONFIG_SLAB_MERGE_DEFAULT would normally cover the zsmalloc caches case too, but was disabled. I agree with the arguments for this change though, so it doesn't depend on the general merging. Acked-by: Vlastimil Babka <vbabka@xxxxxxx> > --- > mm/zsmalloc.c | 87 ++++++++++++++++++++++++++------------------------- > 1 file changed, 44 insertions(+), 43 deletions(-) > > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c > index b42d3545ca856..76d9976442a4a 100644 > --- a/mm/zsmalloc.c > +++ b/mm/zsmalloc.c > @@ -220,8 +220,6 @@ struct zs_pool { > const char *name; > > struct size_class *size_class[ZS_SIZE_CLASSES]; > - struct kmem_cache *handle_cachep; > - struct kmem_cache *zspage_cachep; > > atomic_long_t pages_allocated; > > @@ -289,50 +287,29 @@ static void init_deferred_free(struct zs_pool *pool) {} > static void SetZsPageMovable(struct zs_pool *pool, struct zspage *zspage) {} > #endif > > -static int create_cache(struct zs_pool *pool) > -{ > - pool->handle_cachep = kmem_cache_create("zs_handle", ZS_HANDLE_SIZE, > - 0, 0, NULL); > - if (!pool->handle_cachep) > - return 1; > - > - pool->zspage_cachep = kmem_cache_create("zspage", sizeof(struct zspage), > - 0, 0, NULL); > - if (!pool->zspage_cachep) { > - kmem_cache_destroy(pool->handle_cachep); > - pool->handle_cachep = NULL; > - return 1; > - } > - > - return 0; > -} > +static struct kmem_cache *zs_handle_cache; > +static struct kmem_cache *zspage_cache; > > -static void destroy_cache(struct zs_pool *pool) > +static unsigned long cache_alloc_handle(gfp_t gfp) > { > - kmem_cache_destroy(pool->handle_cachep); > - kmem_cache_destroy(pool->zspage_cachep); > -} > - > -static unsigned long cache_alloc_handle(struct zs_pool *pool, gfp_t gfp) > -{ > - return (unsigned long)kmem_cache_alloc(pool->handle_cachep, > + return (unsigned long)kmem_cache_alloc(zs_handle_cache, > gfp & ~(__GFP_HIGHMEM|__GFP_MOVABLE)); > } > > -static void cache_free_handle(struct zs_pool *pool, unsigned long handle) > +static void cache_free_handle(unsigned long handle) > { > - kmem_cache_free(pool->handle_cachep, (void *)handle); > + kmem_cache_free(zs_handle_cache, (void *)handle); > } > > -static struct zspage *cache_alloc_zspage(struct zs_pool *pool, gfp_t flags) > +static struct zspage *cache_alloc_zspage(gfp_t flags) > { > - return kmem_cache_zalloc(pool->zspage_cachep, > + return kmem_cache_zalloc(zspage_cache, > flags & ~(__GFP_HIGHMEM|__GFP_MOVABLE)); > } > > -static void cache_free_zspage(struct zs_pool *pool, struct zspage *zspage) > +static void cache_free_zspage(struct zspage *zspage) > { > - kmem_cache_free(pool->zspage_cachep, zspage); > + kmem_cache_free(zspage_cache, zspage); > } > > /* pool->lock(which owns the handle) synchronizes races */ > @@ -837,7 +814,7 @@ static void __free_zspage(struct zs_pool *pool, struct size_class *class, > page = next; > } while (page != NULL); > > - cache_free_zspage(pool, zspage); > + cache_free_zspage(zspage); > > class_stat_dec(class, ZS_OBJS_ALLOCATED, class->objs_per_zspage); > atomic_long_sub(class->pages_per_zspage, &pool->pages_allocated); > @@ -950,7 +927,7 @@ static struct zspage *alloc_zspage(struct zs_pool *pool, > { > int i; > struct page *pages[ZS_MAX_PAGES_PER_ZSPAGE]; > - struct zspage *zspage = cache_alloc_zspage(pool, gfp); > + struct zspage *zspage = cache_alloc_zspage(gfp); > > if (!zspage) > return NULL; > @@ -967,7 +944,7 @@ static struct zspage *alloc_zspage(struct zs_pool *pool, > dec_zone_page_state(pages[i], NR_ZSPAGES); > __free_page(pages[i]); > } > - cache_free_zspage(pool, zspage); > + cache_free_zspage(zspage); > return NULL; > } > > @@ -1338,7 +1315,7 @@ unsigned long zs_malloc(struct zs_pool *pool, size_t size, gfp_t gfp) > if (unlikely(size > ZS_MAX_ALLOC_SIZE)) > return (unsigned long)ERR_PTR(-ENOSPC); > > - handle = cache_alloc_handle(pool, gfp); > + handle = cache_alloc_handle(gfp); > if (!handle) > return (unsigned long)ERR_PTR(-ENOMEM); > > @@ -1363,7 +1340,7 @@ unsigned long zs_malloc(struct zs_pool *pool, size_t size, gfp_t gfp) > > zspage = alloc_zspage(pool, class, gfp); > if (!zspage) { > - cache_free_handle(pool, handle); > + cache_free_handle(handle); > return (unsigned long)ERR_PTR(-ENOMEM); > } > > @@ -1441,7 +1418,7 @@ void zs_free(struct zs_pool *pool, unsigned long handle) > free_zspage(pool, class, zspage); > > spin_unlock(&pool->lock); > - cache_free_handle(pool, handle); > + cache_free_handle(handle); > } > EXPORT_SYMBOL_GPL(zs_free); > > @@ -2111,9 +2088,6 @@ struct zs_pool *zs_create_pool(const char *name) > if (!pool->name) > goto err; > > - if (create_cache(pool)) > - goto err; > - > /* > * Iterate reversely, because, size of size_class that we want to use > * for merging should be larger or equal to current size. > @@ -2234,16 +2208,41 @@ void zs_destroy_pool(struct zs_pool *pool) > kfree(class); > } > > - destroy_cache(pool); > kfree(pool->name); > kfree(pool); > } > EXPORT_SYMBOL_GPL(zs_destroy_pool); > > +static void zs_destroy_caches(void) > +{ > + kmem_cache_destroy(zs_handle_cache); > + kmem_cache_destroy(zspage_cache); > + zs_handle_cache = NULL; > + zspage_cache = NULL; > +} > + > +static int zs_create_caches(void) > +{ > + zs_handle_cache = kmem_cache_create("zs_handle", ZS_HANDLE_SIZE, > + 0, 0, NULL); > + zspage_cache = kmem_cache_create("zspage", sizeof(struct zspage), > + 0, 0, NULL); > + > + if (!zs_handle_cache || !zspage_cache) { > + zs_destroy_caches(); > + return -1; > + } > + return 0; > +} > + > static int __init zs_init(void) > { > int ret; > > + ret = zs_create_caches(); > + if (ret) > + goto out; > + > ret = cpuhp_setup_state(CPUHP_MM_ZS_PREPARE, "mm/zsmalloc:prepare", > zs_cpu_prepare, zs_cpu_dead); > if (ret) > @@ -2258,6 +2257,7 @@ static int __init zs_init(void) > return 0; > > out: > + zs_destroy_caches(); > return ret; > } > > @@ -2269,6 +2269,7 @@ static void __exit zs_exit(void) > cpuhp_remove_state(CPUHP_MM_ZS_PREPARE); > > zs_stat_exit(); > + zs_destroy_caches(); > } > > module_init(zs_init);
