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
Remove zswap_entry_cache_create and zswap_entry_cache_destroy and use
kmem_cache_* function directly.
Signed-off-by: Liu Shixin <[email protected]>
---
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
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 <[email protected]>
---
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
If parameter is invalid or no change required, return directly. This can
reduces unnecessary printing.
Signed-off-by: Liu Shixin <[email protected]>
---
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
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 <[email protected]>
---
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 <[email protected]>");
--
2.25.1
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).
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.
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.
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
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.
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.
> @@ -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.
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.
>
> .
>