| Message ID | 20230322102006.780624-3-liushixin2@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Delay the initialization of zswap | expand |
On Wed, Mar 22, 2023 at 10:30 AM Liu Shixin <liushixin2@huawei.com> 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 <liushixin2@huawei.com> > --- > 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 <sjennings@variantweb.net>"); > -- > 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 <liushixin2@huawei.com> 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 <liushixin2@huawei.com> > --- > 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 <liushixin2@huawei.com> >> --- >> 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 <hch@lst.de> 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 <liushixin2@huawei.com> 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
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 <sjennings@variantweb.net>");
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 | 51 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 10 deletions(-)