[PATCH -next v9 0/3] Delay the initialization of zswap

[PATCH -next v9 0/3] 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.

v8->v9: Fix some pattern problem suggested by Christoph.
v7->v8: Do some cleanup. And remove the second patch in v7 which is unrelated
	to the initialization of zswap.
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 (3):
  mm/zswap: remove zswap_entry_cache_{create,destroy} helper function
  mm/zswap: replace zswap_init_{started/failed} with zswap_init_state
  mm/zswap: delay the initialization of zswap

 mm/zswap.c | 122 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 77 insertions(+), 45 deletions(-)

-- 
2.25.1

[PATCH -next v9 1/3] 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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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 v9 2/3] 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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 mm/zswap.c | 55 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 6d2b879f091e..9169c2baee87 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -214,11 +214,13 @@ 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;
+enum zswap_init_type {
+	ZSWAP_UNINIT,
+	ZSWAP_INIT_SUCCEED,
+	ZSWAP_INIT_FAILED
+};
 
-/* fatal error during init */
-static bool zswap_init_failed;
+static enum zswap_init_type zswap_init_state;
 
 /* init completed, but couldn't create the initial pool */
 static bool zswap_has_pool;
@@ -761,21 +763,22 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
 	char *s = strstrip((char *)val);
 	int ret;
 
-	if (zswap_init_failed) {
+	switch (zswap_init_state) {
+	case ZSWAP_UNINIT:
+		/* if this is load-time (pre-init) param setting,
+		 * don't create a pool; that's done during init.
+		 */
+		return param_set_charp(s, kp);
+	case ZSWAP_INIT_SUCCEED:
+		/* no change required */
+		if (!strcmp(s, *(char **)kp->arg) && zswap_has_pool)
+			return 0;
+		break;
+	case 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.
-	 */
-	if (!zswap_init_started)
-		return param_set_charp(s, kp);
-
 	if (!type) {
 		if (!zpool_has_pool(s)) {
 			pr_err("zpool %s not available\n", s);
@@ -864,16 +867,19 @@ static int zswap_zpool_param_set(const char *val,
 static int zswap_enabled_param_set(const char *val,
 				   const struct kernel_param *kp)
 {
-	if (zswap_init_failed) {
+	switch (zswap_init_state) {
+	case ZSWAP_UNINIT:
+		return param_set_bool(val, kp);
+	case ZSWAP_INIT_SUCCEED:
+		if (!zswap_has_pool) {
+			pr_err("can't enable, no pool configured\n");
+			return -ENODEV;
+		} else
+			return param_set_bool(val, kp);
+	case ZSWAP_INIT_FAILED:
 		pr_err("can't enable, initialization failed\n");
 		return -ENODEV;
 	}
-	if (!zswap_has_pool && zswap_init_started) {
-		pr_err("can't enable, no pool configured\n");
-		return -ENODEV;
-	}
-
-	return param_set_bool(val, kp);
 }
 
 /*********************************
@@ -1476,8 +1482,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");
@@ -1518,6 +1522,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:
@@ -1531,7 +1536,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 v9 3/3] mm/zswap: delay the initialization 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 | 71 +++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 17 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 9169c2baee87..af97e8f9d678 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 *,
@@ -222,6 +224,9 @@ enum zswap_init_type {
 
 static enum zswap_init_type 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;
 
@@ -654,7 +659,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;
 
@@ -755,29 +760,43 @@ static void zswap_pool_put(struct zswap_pool *pool)
 * param callbacks
 **********************************/
 
+static bool zswap_pool_changed(const char *s, const struct kernel_param *kp)
+{
+	/* no change required */
+	if (!strcmp(s, *(char **)kp->arg) && zswap_has_pool)
+		return false;
+	return true;
+}
+
 /* val must be a null-terminated string */
 static int __zswap_param_set(const char *val, const struct kernel_param *kp,
 			     char *type, char *compressor)
 {
 	struct zswap_pool *pool, *put_pool = NULL;
 	char *s = strstrip((char *)val);
-	int ret;
+	int ret = 0;
+	bool new_pool = false;
 
+	mutex_lock(&zswap_init_lock);
 	switch (zswap_init_state) {
 	case ZSWAP_UNINIT:
 		/* if this is load-time (pre-init) param setting,
 		 * don't create a pool; that's done during init.
 		 */
-		return param_set_charp(s, kp);
+		ret = param_set_charp(s, kp);
+		break;
 	case ZSWAP_INIT_SUCCEED:
-		/* no change required */
-		if (!strcmp(s, *(char **)kp->arg) && zswap_has_pool)
-			return 0;
+		new_pool = zswap_pool_changed(s, kp);
 		break;
 	case ZSWAP_INIT_FAILED:
 		pr_err("can't set param, initialization failed\n");
-		return -ENODEV;
+		ret = -ENODEV;
 	}
+	mutex_unlock(&zswap_init_lock);
+
+	/* no need to create a new pool, return directly */
+	if (!new_pool)
+		return ret;
 
 	if (!type) {
 		if (!zpool_has_pool(s)) {
@@ -867,19 +886,30 @@ static int zswap_zpool_param_set(const char *val,
 static int zswap_enabled_param_set(const char *val,
 				   const struct kernel_param *kp)
 {
+	int ret = -ENODEV;
+
+	/* if this is load-time (pre-init) param setting, only set param. */
+	if (system_state != SYSTEM_RUNNING)
+		return param_set_bool(val, kp);
+
+	mutex_lock(&zswap_init_lock);
 	switch (zswap_init_state) {
 	case ZSWAP_UNINIT:
-		return param_set_bool(val, kp);
+		if (zswap_setup())
+			break;
+		fallthrough;
 	case ZSWAP_INIT_SUCCEED:
-		if (!zswap_has_pool) {
+		if (!zswap_has_pool)
 			pr_err("can't enable, no pool configured\n");
-			return -ENODEV;
-		} else
-			return param_set_bool(val, kp);
+		else
+			ret = param_set_bool(val, kp);
+		break;
 	case ZSWAP_INIT_FAILED:
 		pr_err("can't enable, initialization failed\n");
-		return -ENODEV;
 	}
+	mutex_unlock(&zswap_init_lock);
+
+	return ret;
 }
 
 /*********************************
@@ -1437,7 +1467,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;
@@ -1468,7 +1498,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;
 }
@@ -1477,7 +1507,7 @@ 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;
@@ -1540,8 +1570,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 v9 3/3] mm/zswap: delay the initialization of zswap

[Christoph Hellwig]

On Tue, Apr 11, 2023 at 05:36:32PM +0800, Liu Shixin wrote:
> 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>

Looks good:

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

Re: [PATCH -next v9 0/3] Delay the initialization of zswap

[Chris Li]

Hi Shixing,

On Tue, Apr 11, 2023 at 05:36:29PM +0800, Liu Shixin wrote:
> 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.

Sorry I am late to this discussion. I notice you are already at V9.
Anyway, I am curious how much of the 18MB was came from the zswap_pool
alone and how much of it came from the other part of the initialization.

If it is the zswap_pool alone, it means that we can have a smaller patch
to get most of your 18M back.

I also notice you move a lot of __init function back to normal functions.
That would mean those functions wouldn't be able to drop after the
initialization phase. That is another reason to move less of the initialization
function.

Chris

Re: [PATCH -next v9 0/3] Delay the initialization of zswap

[Liu Shixin]

On 2023/5/4 8:11, Chris Li wrote:
> Hi Shixing,
>
> On Tue, Apr 11, 2023 at 05:36:29PM +0800, Liu Shixin wrote:
>> 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.
> Sorry I am late to this discussion. I notice you are already at V9.
> Anyway, I am curious how much of the 18MB was came from the zswap_pool
> alone and how much of it came from the other part of the initialization.
>
> If it is the zswap_pool alone, it means that we can have a smaller patch
> to get most of your 18M back.
You're right, the most came from zswap_pool.
>
> I also notice you move a lot of __init function back to normal functions.
> That would mean those functions wouldn't be able to drop after the
> initialization phase. That is another reason to move less of the initialization
> function.
Thanks for your advice. I've thought about it before, but I thought there is less impact
for the size of kernel, so I didn't do it.
>
> Chris
>
> .
>

Re: [PATCH -next v9 0/3] Delay the initialization of zswap

[Chris Li]

On Thu, May 04, 2023 at 03:11:05PM +0800, Liu Shixin wrote:
> >
> > If it is the zswap_pool alone, it means that we can have a smaller patch
> > to get most of your 18M back.
> You're right, the most came from zswap_pool.

Thanks for the confirmation.

> > I also notice you move a lot of __init function back to normal functions.
> > That would mean those functions wouldn't be able to drop after the
> > initialization phase. That is another reason to move less of the initialization
> > function.
> Thanks for your advice. I've thought about it before, but I thought there is less impact
> for the size of kernel, so I didn't do it.

Let's first agree on the hypothetical patch that only delaying zswap_pool would
have the benefit over V9 on:
- smaller patch, less invasive.
- less kernel text area due to more __init function got free after initialization.

If we can reach that agreement, then we can discuss how we can get there.

I think there is a possibility that the delay initialization of zswap_pool
can fall into the "zswap_has_pool = false" case, so you don't need to have
the initialization mutex.  Simpler.

I have my selfish reason as well. I have a much larger pending patch against
the zswap code so the smaller patch would mean less conflict for me.

I am guilty of giving this feedback late. If you come up with a V10, I will be glad
to review it. Or, if you prefer, I can come up with the smaller patch for you
to review as well. What do you say?

Chris

Re: [PATCH -next v9 0/3] Delay the initialization of zswap

[Liu Shixin]

On 2023/5/4 22:53, Chris Li wrote:
> On Thu, May 04, 2023 at 03:11:05PM +0800, Liu Shixin wrote:
>>> If it is the zswap_pool alone, it means that we can have a smaller patch
>>> to get most of your 18M back.
>> You're right, the most came from zswap_pool.
> Thanks for the confirmation.
>
>>> I also notice you move a lot of __init function back to normal functions.
>>> That would mean those functions wouldn't be able to drop after the
>>> initialization phase. That is another reason to move less of the initialization
>>> function.
>> Thanks for your advice. I've thought about it before, but I thought there is less impact
>> for the size of kernel, so I didn't do it.
> Let's first agree on the hypothetical patch that only delaying zswap_pool would
> have the benefit over V9 on:
> - smaller patch, less invasive.
> - less kernel text area due to more __init function got free after initialization.
>
> If we can reach that agreement, then we can discuss how we can get there.
>
> I think there is a possibility that the delay initialization of zswap_pool
> can fall into the "zswap_has_pool = false" case, so you don't need to have
> the initialization mutex.  Simpler.
>
> I have my selfish reason as well. I have a much larger pending patch against
> the zswap code so the smaller patch would mean less conflict for me.
>
> I am guilty of giving this feedback late. If you come up with a V10, I will be glad
> to review it. Or, if you prefer, I can come up with the smaller patch for you
> to review as well. What do you say?
You can add a pre-patch to modify it before your patch. Thanks.
>
> Chris
>
> .
>

Re: [PATCH -next v9 0/3] Delay the initialization of zswap

[Chris Li]

Hi Shixin,

On Mon, May 08, 2023 at 09:37:23AM +0800, Liu Shixin wrote:
> > I am guilty of giving this feedback late. If you come up with a V10, I will be glad
> > to review it. Or, if you prefer, I can come up with the smaller patch for you
> > to review as well. What do you say?
> You can add a pre-patch to modify it before your patch. Thanks.

Will do. I notice your patch has already been merged, that is the
only way to move forward anyway.

I will CC you for review when I send the patch out.

Chris