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.
v5->v6: Simplify the code and delete the patches about frontswap suggested
by Christoph.
Liu Shixin (2):
mm/zswap: skip invalid or unchanged parameter
mm/zswap: delay the initializaton of zswap
mm/zswap.c | 60 +++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 50 insertions(+), 10 deletions(-)
--
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 | 51 +++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 41 insertions(+), 10 deletions(-)
diff --git a/mm/zswap.c b/mm/zswap.c
index 09fa956920fa..3aed3b26524a 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 bool zswap_init_started;
/* fatal error during init */
static bool zswap_init_failed;
+/* 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;
@@ -272,13 +277,13 @@ static void zswap_update_total_size(void)
**********************************/
static struct kmem_cache *zswap_entry_cache;
-static int __init zswap_entry_cache_create(void)
+static int 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)
+static void zswap_entry_cache_destroy(void)
{
kmem_cache_destroy(zswap_entry_cache);
}
@@ -663,7 +668,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;
@@ -784,8 +789,15 @@ 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)
- return param_set_charp(s, kp);
+ if (!zswap_init_started) {
+ mutex_lock(&zswap_init_lock);
+ if (!zswap_init_started) {
+ 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)) {
@@ -884,6 +896,15 @@ static int zswap_enabled_param_set(const char *val,
if (res == *(bool *)kp->arg)
return 0;
+ if (!zswap_init_started && (system_state == SYSTEM_RUNNING)) {
+ mutex_lock(&zswap_init_lock);
+ if (zswap_setup()) {
+ mutex_unlock(&zswap_init_lock);
+ return -ENODEV;
+ }
+ mutex_unlock(&zswap_init_lock);
+ }
+
if (zswap_init_failed) {
pr_err("can't enable, initialization failed\n");
return -ENODEV;
@@ -1451,7 +1472,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;
@@ -1482,7 +1503,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;
}
@@ -1491,12 +1512,13 @@ 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;
- zswap_init_started = true;
+ if (zswap_init_started)
+ return 0;
if (zswap_entry_cache_create()) {
pr_err("entry cache creation failed\n");
@@ -1537,6 +1559,7 @@ static int __init init_zswap(void)
goto destroy_wq;
if (zswap_debugfs_init())
pr_warn("debugfs initialization failed\n");
+ zswap_init_started = true;
return 0;
destroy_wq:
@@ -1551,11 +1574,19 @@ static int __init init_zswap(void)
cache_fail:
/* if built-in, we aren't unloaded on failure; don't allow use */
zswap_init_failed = true;
+ zswap_init_started = true;
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
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 | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/mm/zswap.c b/mm/zswap.c
index 2f0ebd8bc620..09fa956920fa 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -875,6 +875,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
On Wed, Mar 22, 2023 at 10:30 AM Liu Shixin <[email protected]> wrote:
>
> Since some users may not use zswap, the zswap_pool is wasted. Save memory
> by delaying the initialization of zswap until enabled.
To be honest, I'm not a huge fan of this. Would enabling zswap module
build instead solve your problem?
~Vitaly
> Signed-off-by: Liu Shixin <[email protected]>
> ---
> mm/zswap.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 41 insertions(+), 10 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 09fa956920fa..3aed3b26524a 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 bool zswap_init_started;
> /* fatal error during init */
> static bool zswap_init_failed;
>
> +/* 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;
>
> @@ -272,13 +277,13 @@ static void zswap_update_total_size(void)
> **********************************/
> static struct kmem_cache *zswap_entry_cache;
>
> -static int __init zswap_entry_cache_create(void)
> +static int 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)
> +static void zswap_entry_cache_destroy(void)
> {
> kmem_cache_destroy(zswap_entry_cache);
> }
> @@ -663,7 +668,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;
>
> @@ -784,8 +789,15 @@ 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)
> - return param_set_charp(s, kp);
> + if (!zswap_init_started) {
> + mutex_lock(&zswap_init_lock);
> + if (!zswap_init_started) {
> + 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)) {
> @@ -884,6 +896,15 @@ static int zswap_enabled_param_set(const char *val,
> if (res == *(bool *)kp->arg)
> return 0;
>
> + if (!zswap_init_started && (system_state == SYSTEM_RUNNING)) {
> + mutex_lock(&zswap_init_lock);
> + if (zswap_setup()) {
> + mutex_unlock(&zswap_init_lock);
> + return -ENODEV;
> + }
> + mutex_unlock(&zswap_init_lock);
> + }
> +
> if (zswap_init_failed) {
> pr_err("can't enable, initialization failed\n");
> return -ENODEV;
> @@ -1451,7 +1472,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;
> @@ -1482,7 +1503,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;
> }
> @@ -1491,12 +1512,13 @@ 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;
>
> - zswap_init_started = true;
> + if (zswap_init_started)
> + return 0;
>
> if (zswap_entry_cache_create()) {
> pr_err("entry cache creation failed\n");
> @@ -1537,6 +1559,7 @@ static int __init init_zswap(void)
> goto destroy_wq;
> if (zswap_debugfs_init())
> pr_warn("debugfs initialization failed\n");
> + zswap_init_started = true;
> return 0;
>
> destroy_wq:
> @@ -1551,11 +1574,19 @@ static int __init init_zswap(void)
> cache_fail:
> /* if built-in, we aren't unloaded on failure; don't allow use */
> zswap_init_failed = true;
> + zswap_init_started = true;
> 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 Wed, Mar 22, 2023 at 06:17:12PM +0100, Vitaly Wool wrote:
> On Wed, Mar 22, 2023 at 10:30 AM Liu Shixin <[email protected]> wrote:
> >
> > Since some users may not use zswap, the zswap_pool is wasted. Save memory
> > by delaying the initialization of zswap until enabled.
>
> To be honest, I'm not a huge fan of this. Would enabling zswap module
> build instead solve your problem?
making zswap build modular would be a mess. It is core MM infrastructure
and now we'd need to start dealing with adding and removing it at
runtime as well as module refcounting.
On Wed, Mar 22, 2023 at 06:20:06PM +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 <[email protected]>
> ---
> mm/zswap.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 41 insertions(+), 10 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 09fa956920fa..3aed3b26524a 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 bool zswap_init_started;
> /* fatal error during init */
> static bool zswap_init_failed;
>
> +/* 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;
>
> @@ -272,13 +277,13 @@ static void zswap_update_total_size(void)
> **********************************/
> static struct kmem_cache *zswap_entry_cache;
>
> -static int __init zswap_entry_cache_create(void)
> +static int zswap_entry_cache_create(void)
> {
> zswap_entry_cache = KMEM_CACHE(zswap_entry, 0);
> return zswap_entry_cache == NULL;
> }
Please add a cleanup patch to remove this helper first, it just
massivel confuses the reader.
> -static void __init zswap_entry_cache_destroy(void)
> +static void zswap_entry_cache_destroy(void)
> {
> kmem_cache_destroy(zswap_entry_cache);
> }
Same here.
> @@ -663,7 +668,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;
>
> @@ -784,8 +789,15 @@ 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)
> - return param_set_charp(s, kp);
> + if (!zswap_init_started) {
> + mutex_lock(&zswap_init_lock);
> + if (!zswap_init_started) {
> + ret = param_set_charp(s, kp);
> + mutex_unlock(&zswap_init_lock);
> + return ret;
> + }
> + mutex_unlock(&zswap_init_lock);
> + }
Just take the lock around the whole function. No need to micro-optimize
setting a kernel paramter.
> @@ -884,6 +896,15 @@ static int zswap_enabled_param_set(const char *val,
> if (res == *(bool *)kp->arg)
> return 0;
>
> + if (!zswap_init_started && (system_state == SYSTEM_RUNNING)) {
No need for the inner braces. But directly looking at
SYSTEM_RUNNING, especially without a comment is a bit of a mess.
Is there any better way to deal with this?
Also the zswap_init_started variable name has always been a bit
confusing. If everything around it takes zswap_init_lock now,
it can be replaced with a check for successful zswap initialization
as all the initializtion is covered by the lock. That would really
help to clean up the code.
> +static int zswap_debugfs_init(void)
> {
> if (!debugfs_initialized())
> return -ENODEV;
> @@ -1482,7 +1503,7 @@ static int __init zswap_debugfs_init(void)
> return 0;
> }
> #else
> -static int __init zswap_debugfs_init(void)
> +static int zswap_debugfs_init(void)
Is there any reason to not just always initialize debugfs and
only defer the expensive allocations?
On 2023/3/23 16:04, Christoph Hellwig wrote:
> On Wed, Mar 22, 2023 at 06:20:06PM +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 <[email protected]>
>> ---
>> mm/zswap.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 41 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/zswap.c b/mm/zswap.c
>> index 09fa956920fa..3aed3b26524a 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 bool zswap_init_started;
>> /* fatal error during init */
>> static bool zswap_init_failed;
>>
>> +/* 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;
>>
>> @@ -272,13 +277,13 @@ static void zswap_update_total_size(void)
>> **********************************/
>> static struct kmem_cache *zswap_entry_cache;
>>
>> -static int __init zswap_entry_cache_create(void)
>> +static int zswap_entry_cache_create(void)
>> {
>> zswap_entry_cache = KMEM_CACHE(zswap_entry, 0);
>> return zswap_entry_cache == NULL;
>> }
> Please add a cleanup patch to remove this helper first, it just
> massivel confuses the reader.
I will, thanks.
>
>> -static void __init zswap_entry_cache_destroy(void)
>> +static void zswap_entry_cache_destroy(void)
>> {
>> kmem_cache_destroy(zswap_entry_cache);
>> }
> Same here.
>
>> @@ -663,7 +668,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;
>>
>> @@ -784,8 +789,15 @@ 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)
>> - return param_set_charp(s, kp);
>> + if (!zswap_init_started) {
>> + mutex_lock(&zswap_init_lock);
>> + if (!zswap_init_started) {
>> + ret = param_set_charp(s, kp);
>> + mutex_unlock(&zswap_init_lock);
>> + return ret;
>> + }
>> + mutex_unlock(&zswap_init_lock);
>> + }
> Just take the lock around the whole function. No need to micro-optimize
> setting a kernel paramter.
I will, thanks.
>
>> @@ -884,6 +896,15 @@ static int zswap_enabled_param_set(const char *val,
>> if (res == *(bool *)kp->arg)
>> return 0;
>>
>> + if (!zswap_init_started && (system_state == SYSTEM_RUNNING)) {
> No need for the inner braces. But directly looking at
> SYSTEM_RUNNING, especially without a comment is a bit of a mess.
> Is there any better way to deal with this?
I have no idea about better way.
>
> Also the zswap_init_started variable name has always been a bit
> confusing. If everything around it takes zswap_init_lock now,
> it can be replaced with a check for successful zswap initialization
> as all the initializtion is covered by the lock. That would really
> help to clean up the code.
I will, thanks.
>
>> +static int zswap_debugfs_init(void)
>> {
>> if (!debugfs_initialized())
>> return -ENODEV;
>> @@ -1482,7 +1503,7 @@ static int __init zswap_debugfs_init(void)
>> return 0;
>> }
>> #else
>> -static int __init zswap_debugfs_init(void)
>> +static int zswap_debugfs_init(void)
> Is there any reason to not just always initialize debugfs and
> only defer the expensive allocations?
It seems there is no need to initialize debugfs if zswap is not used.
>
> .
>
On Thu, Mar 23, 2023 at 8:59 AM Christoph Hellwig <[email protected]> wrote:
>
> On Wed, Mar 22, 2023 at 06:17:12PM +0100, Vitaly Wool wrote:
> > On Wed, Mar 22, 2023 at 10:30 AM Liu Shixin <[email protected]> wrote:
> > >
> > > Since some users may not use zswap, the zswap_pool is wasted. Save memory
> > > by delaying the initialization of zswap until enabled.
> >
> > To be honest, I'm not a huge fan of this. Would enabling zswap module
> > build instead solve your problem?
>
> making zswap build modular would be a mess. It is core MM infrastructure
> and now we'd need to start dealing with adding and removing it at
> runtime as well as module refcounting.
Since when is it a core MM infrastructure? It is basically just a
frontswap frontend.
~Vitaly