| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 4 Jun 2024 22:54:59 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Yosry Ahmed <yosryahmed@...gle.com>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: Minchan Kim <minchan@...nel.org>,
Sergey Senozhatsky <senozhatsky@...omium.org>,
David Rientjes <rientjes@...gle.com>, Christoph Lameter <cl@...ux.com>,
Erhard Furtner <erhard_f@...lbox.org>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: zsmalloc: share slab caches for all zsmalloc zpools
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@...gle.com>
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@...e.cz>
> ---
> 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);
Powered by blists - more mailing lists