[PATCH -next v7 0/4] Delay the initialization of zswap

[PATCH -next v7 0/4] Delay the initialization of zswap

[Liu Shixin]

In the initialization of zswap, about 18MB memory will be allocated for
zswap_pool. Since some users may not use zswap, the zswap_pool is wasted.
Save memory by delaying the initialization of zswap until enabled.

v6->v7: Add two new patch[1,3] to cleanup the code. And cover zswap_init_*
	parameter by zswap_init_lock to protect against conflicts.
v5->v6: Simplify the code and delete the patches about frontswap suggested
	by Christoph.

Liu Shixin (4):
  mm/zswap: remove zswap_entry_cache_{create,destroy} helper function
  mm/zswap: skip invalid or unchanged parameter
  mm/zswap: replace zswap_init_{started/failed} with zswap_init_state
  mm/zswap: delay the initializaton of zswap

 mm/zswap.c | 94 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 61 insertions(+), 33 deletions(-)

-- 
2.25.1

[PATCH -next v7 1/4] mm/zswap: remove zswap_entry_cache_{create,destroy} helper function

[Liu Shixin]

Remove zswap_entry_cache_create and zswap_entry_cache_destroy and use
kmem_cache_* function directly.

Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 mm/zswap.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 2f0ebd8bc620..6d2b879f091e 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -272,17 +272,6 @@ static void zswap_update_total_size(void)
 **********************************/
 static struct kmem_cache *zswap_entry_cache;
 
-static int __init zswap_entry_cache_create(void)
-{
-	zswap_entry_cache = KMEM_CACHE(zswap_entry, 0);
-	return zswap_entry_cache == NULL;
-}
-
-static void __init zswap_entry_cache_destroy(void)
-{
-	kmem_cache_destroy(zswap_entry_cache);
-}
-
 static struct zswap_entry *zswap_entry_cache_alloc(gfp_t gfp)
 {
 	struct zswap_entry *entry;
@@ -1489,7 +1478,8 @@ static int __init init_zswap(void)
 
 	zswap_init_started = true;
 
-	if (zswap_entry_cache_create()) {
+	zswap_entry_cache = KMEM_CACHE(zswap_entry, 0);
+	if (!zswap_entry_cache) {
 		pr_err("entry cache creation failed\n");
 		goto cache_fail;
 	}
@@ -1538,7 +1528,7 @@ static int __init init_zswap(void)
 hp_fail:
 	cpuhp_remove_state(CPUHP_MM_ZSWP_MEM_PREPARE);
 dstmem_fail:
-	zswap_entry_cache_destroy();
+	kmem_cache_destroy(zswap_entry_cache);
 cache_fail:
 	/* if built-in, we aren't unloaded on failure; don't allow use */
 	zswap_init_failed = true;
-- 
2.25.1

[PATCH -next v7 2/4] mm/zswap: skip invalid or unchanged parameter

[Liu Shixin]

If parameter is invalid or no change required, return directly. This can
reduces unnecessary printing.

Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 mm/zswap.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 6d2b879f091e..d2adc1ffe47d 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -761,15 +761,15 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
 	char *s = strstrip((char *)val);
 	int ret;
 
+	/* no change required */
+	if (!strcmp(s, *(char **)kp->arg) && zswap_has_pool)
+		return 0;
+
 	if (zswap_init_failed) {
 		pr_err("can't set param, initialization failed\n");
 		return -ENODEV;
 	}
 
-	/* no change required */
-	if (!strcmp(s, *(char **)kp->arg) && zswap_has_pool)
-		return 0;
-
 	/* if this is load-time (pre-init) param setting,
 	 * don't create a pool; that's done during init.
 	 */
@@ -864,6 +864,15 @@ static int zswap_zpool_param_set(const char *val,
 static int zswap_enabled_param_set(const char *val,
 				   const struct kernel_param *kp)
 {
+	bool res;
+
+	if (kstrtobool(val, &res))
+		return -EINVAL;
+
+	/* no change required */
+	if (res == *(bool *)kp->arg)
+		return 0;
+
 	if (zswap_init_failed) {
 		pr_err("can't enable, initialization failed\n");
 		return -ENODEV;
-- 
2.25.1

[PATCH -next v7 3/4] mm/zswap: replace zswap_init_{started/failed} with zswap_init_state

[Liu Shixin]

The zswap_init_started variable name has a bit confusing. Actually, there
are three state: uninitialized, initial failed and initial succeed. Add
a new variable zswap_init_state to replace zswap_init_{started/failed}.

Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 mm/zswap.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index d2adc1ffe47d..9eda48c8b8dc 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -214,11 +214,11 @@ static DEFINE_SPINLOCK(zswap_pools_lock);
 /* pool counter to provide unique names to zpool */
 static atomic_t zswap_pools_count = ATOMIC_INIT(0);
 
-/* used by param callback function */
-static bool zswap_init_started;
+#define ZSWAP_UNINIT		0x0
+#define ZSWAP_INIT_SUCCEED	0x1
+#define ZSWAP_INIT_FAILED	0x2
 
-/* fatal error during init */
-static bool zswap_init_failed;
+static int zswap_init_state;
 
 /* init completed, but couldn't create the initial pool */
 static bool zswap_has_pool;
@@ -765,7 +765,7 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
 	if (!strcmp(s, *(char **)kp->arg) && zswap_has_pool)
 		return 0;
 
-	if (zswap_init_failed) {
+	if (zswap_init_state == ZSWAP_INIT_FAILED) {
 		pr_err("can't set param, initialization failed\n");
 		return -ENODEV;
 	}
@@ -773,7 +773,7 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
 	/* if this is load-time (pre-init) param setting,
 	 * don't create a pool; that's done during init.
 	 */
-	if (!zswap_init_started)
+	if (zswap_init_state == ZSWAP_UNINIT)
 		return param_set_charp(s, kp);
 
 	if (!type) {
@@ -873,11 +873,11 @@ static int zswap_enabled_param_set(const char *val,
 	if (res == *(bool *)kp->arg)
 		return 0;
 
-	if (zswap_init_failed) {
+	if (zswap_init_state == ZSWAP_INIT_FAILED) {
 		pr_err("can't enable, initialization failed\n");
 		return -ENODEV;
 	}
-	if (!zswap_has_pool && zswap_init_started) {
+	if (!zswap_has_pool && (zswap_init_state == ZSWAP_INIT_SUCCEED)) {
 		pr_err("can't enable, no pool configured\n");
 		return -ENODEV;
 	}
@@ -1485,8 +1485,6 @@ static int __init init_zswap(void)
 	struct zswap_pool *pool;
 	int ret;
 
-	zswap_init_started = true;
-
 	zswap_entry_cache = KMEM_CACHE(zswap_entry, 0);
 	if (!zswap_entry_cache) {
 		pr_err("entry cache creation failed\n");
@@ -1527,6 +1525,7 @@ static int __init init_zswap(void)
 		goto destroy_wq;
 	if (zswap_debugfs_init())
 		pr_warn("debugfs initialization failed\n");
+	zswap_init_state = ZSWAP_INIT_SUCCEED;
 	return 0;
 
 destroy_wq:
@@ -1540,7 +1539,7 @@ static int __init init_zswap(void)
 	kmem_cache_destroy(zswap_entry_cache);
 cache_fail:
 	/* if built-in, we aren't unloaded on failure; don't allow use */
-	zswap_init_failed = true;
+	zswap_init_state = ZSWAP_INIT_FAILED;
 	zswap_enabled = false;
 	return -ENOMEM;
 }
-- 
2.25.1

[PATCH -next v7 4/4] mm/zswap: delay the initializaton of zswap

[Liu Shixin]

Since some users may not use zswap, the zswap_pool is wasted. Save memory
by delaying the initialization of zswap until enabled.

Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 mm/zswap.c | 44 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 9eda48c8b8dc..af98bd8d4b79 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -81,6 +81,8 @@ static bool zswap_pool_reached_full;
 
 #define ZSWAP_PARAM_UNSET ""
 
+static int zswap_setup(void);
+
 /* Enable/disable zswap */
 static bool zswap_enabled = IS_ENABLED(CONFIG_ZSWAP_DEFAULT_ON);
 static int zswap_enabled_param_set(const char *,
@@ -220,6 +222,9 @@ static atomic_t zswap_pools_count = ATOMIC_INIT(0);
 
 static int zswap_init_state;
 
+/* used to ensure the integrity of initialization */
+static DEFINE_MUTEX(zswap_init_lock);
+
 /* init completed, but couldn't create the initial pool */
 static bool zswap_has_pool;
 
@@ -652,7 +657,7 @@ static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
 	return NULL;
 }
 
-static __init struct zswap_pool *__zswap_pool_create_fallback(void)
+static struct zswap_pool *__zswap_pool_create_fallback(void)
 {
 	bool has_comp, has_zpool;
 
@@ -765,16 +770,22 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
 	if (!strcmp(s, *(char **)kp->arg) && zswap_has_pool)
 		return 0;
 
+	mutex_lock(&zswap_init_lock);
 	if (zswap_init_state == ZSWAP_INIT_FAILED) {
 		pr_err("can't set param, initialization failed\n");
+		mutex_unlock(&zswap_init_lock);
 		return -ENODEV;
 	}
 
 	/* if this is load-time (pre-init) param setting,
 	 * don't create a pool; that's done during init.
 	 */
-	if (zswap_init_state == ZSWAP_UNINIT)
-		return param_set_charp(s, kp);
+	if (zswap_init_state == ZSWAP_UNINIT) {
+		ret = param_set_charp(s, kp);
+		mutex_unlock(&zswap_init_lock);
+		return ret;
+	}
+	mutex_unlock(&zswap_init_lock);
 
 	if (!type) {
 		if (!zpool_has_pool(s)) {
@@ -873,14 +884,23 @@ static int zswap_enabled_param_set(const char *val,
 	if (res == *(bool *)kp->arg)
 		return 0;
 
+	mutex_lock(&zswap_init_lock);
+	if (system_state == SYSTEM_RUNNING && zswap_setup()) {
+		mutex_unlock(&zswap_init_lock);
+		return -ENODEV;
+	}
+
 	if (zswap_init_state == ZSWAP_INIT_FAILED) {
 		pr_err("can't enable, initialization failed\n");
+		mutex_unlock(&zswap_init_lock);
 		return -ENODEV;
 	}
 	if (!zswap_has_pool && (zswap_init_state == ZSWAP_INIT_SUCCEED)) {
 		pr_err("can't enable, no pool configured\n");
+		mutex_unlock(&zswap_init_lock);
 		return -ENODEV;
 	}
+	mutex_unlock(&zswap_init_lock);
 
 	return param_set_bool(val, kp);
 }
@@ -1440,7 +1460,7 @@ static const struct frontswap_ops zswap_frontswap_ops = {
 
 static struct dentry *zswap_debugfs_root;
 
-static int __init zswap_debugfs_init(void)
+static int zswap_debugfs_init(void)
 {
 	if (!debugfs_initialized())
 		return -ENODEV;
@@ -1471,7 +1491,7 @@ static int __init zswap_debugfs_init(void)
 	return 0;
 }
 #else
-static int __init zswap_debugfs_init(void)
+static int zswap_debugfs_init(void)
 {
 	return 0;
 }
@@ -1480,11 +1500,14 @@ static int __init zswap_debugfs_init(void)
 /*********************************
 * module init and exit
 **********************************/
-static int __init init_zswap(void)
+static int zswap_setup(void)
 {
 	struct zswap_pool *pool;
 	int ret;
 
+	if (zswap_init_state != ZSWAP_UNINIT)
+		return 0;
+
 	zswap_entry_cache = KMEM_CACHE(zswap_entry, 0);
 	if (!zswap_entry_cache) {
 		pr_err("entry cache creation failed\n");
@@ -1543,8 +1566,15 @@ static int __init init_zswap(void)
 	zswap_enabled = false;
 	return -ENOMEM;
 }
+
+static int __init zswap_init(void)
+{
+	if (!zswap_enabled)
+		return 0;
+	return zswap_setup();
+}
 /* must be late so crypto has time to come up */
-late_initcall(init_zswap);
+late_initcall(zswap_init);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Seth Jennings <sjennings@variantweb.net>");
-- 
2.25.1

Re: [PATCH -next v7 2/4] mm/zswap: skip invalid or unchanged parameter

[Sergey Senozhatsky]

On (23/03/25 15:14), Liu Shixin wrote:
> +++ b/mm/zswap.c
> @@ -761,15 +761,15 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
>  	char *s = strstrip((char *)val);
>  	int ret;
>  
> +	/* no change required */
> +	if (!strcmp(s, *(char **)kp->arg) && zswap_has_pool)
> +		return 0;
> +
>  	if (zswap_init_failed) {
>  		pr_err("can't set param, initialization failed\n");
>  		return -ENODEV;
>  	}
>  
> -	/* no change required */
> -	if (!strcmp(s, *(char **)kp->arg) && zswap_has_pool)
> -		return 0;

I probably would suggest to simply remove `*(char **)kp->arg`
from zswap code entirely, it doesn't solve any real problem
(as far as I can tell).

Re: [PATCH -next v7 2/4] mm/zswap: skip invalid or unchanged parameter

[Sergey Senozhatsky]

On (23/03/25 15:14), Liu Shixin wrote:
>  static int zswap_enabled_param_set(const char *val,
>  				   const struct kernel_param *kp)
>  {
> +	bool res;
> +
> +	if (kstrtobool(val, &res))
> +		return -EINVAL;
> +
> +	/* no change required */
> +	if (res == *(bool *)kp->arg)
> +		return 0;

Bool kernel param can be any of these letters 'YyTt1NnFf0'. Doing things
to kp->arg outside of kernel/params.c is not going to be easy, let's not
even try.

Re: [PATCH -next v7 2/4] mm/zswap: skip invalid or unchanged parameter

[Sergey Senozhatsky]

On (23/03/26 12:17), Sergey Senozhatsky wrote:
> On (23/03/25 15:14), Liu Shixin wrote:
> >  static int zswap_enabled_param_set(const char *val,
> >  				   const struct kernel_param *kp)
> >  {
> > +	bool res;
> > +
> > +	if (kstrtobool(val, &res))
> > +		return -EINVAL;
> > +
> > +	/* no change required */
> > +	if (res == *(bool *)kp->arg)
> > +		return 0;
> 
> Bool kernel param can be any of these letters 'YyTt1NnFf0'. Doing things
> to kp->arg outside of kernel/params.c is not going to be easy, let's not
> even try.

Please disregard my previous email. kp->arg is always true or false
at this point. I'd still prefer to not do kp->arg in zswap.

Re: [PATCH -next v7 1/4] mm/zswap: remove zswap_entry_cache_{create,destroy} helper function

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH -next v7 2/4] mm/zswap: skip invalid or unchanged parameter

[Christoph Hellwig]

On Sun, Mar 26, 2023 at 01:53:27PM +0900, Sergey Senozhatsky wrote:
> > > +	if (kstrtobool(val, &res))
> > > +		return -EINVAL;
> > > +
> > > +	/* no change required */
> > > +	if (res == *(bool *)kp->arg)
> > > +		return 0;
> > 
> > Bool kernel param can be any of these letters 'YyTt1NnFf0'. Doing things
> > to kp->arg outside of kernel/params.c is not going to be easy, let's not
> > even try.
> 
> Please disregard my previous email. kp->arg is always true or false
> at this point. I'd still prefer to not do kp->arg in zswap.

The whole parameter handling in zswap is a mess and I don't really
have a good idea how to solve it all.

But for this "paramter not changed" case I think we can helper a lot
by adding a core moduleparam.h helper to encapsule it.  I.e.:

static inline bool param_bool_unchanged(bool val,
		const struct kernel_param *kp)

	return val == *(bool *)kp->arg);
}

and at least keep it out of zswap.

Re: [PATCH -next v7 3/4] mm/zswap: replace zswap_init_{started/failed} with zswap_init_state

[Christoph Hellwig]

On Sat, Mar 25, 2023 at 03:14:19PM +0800, Liu Shixin wrote:
> -/* used by param callback function */
> -static bool zswap_init_started;
> +#define ZSWAP_UNINIT		0x0
> +#define ZSWAP_INIT_SUCCEED	0x1
> +#define ZSWAP_INIT_FAILED	0x2

This should be a (named) enum.

Re: [PATCH -next v7 4/4] mm/zswap: delay the initializaton of zswap

[Christoph Hellwig]

> @@ -1480,11 +1500,14 @@ static int __init zswap_debugfs_init(void)
>  /*********************************
>  * module init and exit
>  **********************************/
> -static int __init init_zswap(void)
> +static int zswap_setup(void)
>  {
>  	struct zswap_pool *pool;
>  	int ret;
>  
> +	if (zswap_init_state != ZSWAP_UNINIT)
> +		return 0;

I feel like doing this in zswap_enabled_param_set would be a lot cleaner.
With that we could do a switch on the possible enum values for
zswap_init_state there, and that is a good way to explain the possible
outcomes.

Re: [PATCH -next v7 2/4] mm/zswap: skip invalid or unchanged parameter

[Liu Shixin]

On 2023/3/27 7:25, Christoph Hellwig wrote:
> On Sun, Mar 26, 2023 at 01:53:27PM +0900, Sergey Senozhatsky wrote:
>>>> +	if (kstrtobool(val, &res))
>>>> +		return -EINVAL;
>>>> +
>>>> +	/* no change required */
>>>> +	if (res == *(bool *)kp->arg)
>>>> +		return 0;
>>> Bool kernel param can be any of these letters 'YyTt1NnFf0'. Doing things
>>> to kp->arg outside of kernel/params.c is not going to be easy, let's not
>>> even try.
>> Please disregard my previous email. kp->arg is always true or false
>> at this point. I'd still prefer to not do kp->arg in zswap.
> The whole parameter handling in zswap is a mess and I don't really
> have a good idea how to solve it all.
>
> But for this "paramter not changed" case I think we can helper a lot
> by adding a core moduleparam.h helper to encapsule it.  I.e.:
>
> static inline bool param_bool_unchanged(bool val,
> 		const struct kernel_param *kp)
>
> 	return val == *(bool *)kp->arg);
> }
>
> and at least keep it out of zswap.
Thanks for your advice. I will try this way.
>
> .
>