* [PATCH] mm/zsmalloc: don't fail if can't create debugfs info @ 2016-04-28 15:36 Dan Streetman 2016-04-28 22:07 ` Andrew Morton 2016-05-03 2:18 ` Ganesh Mahendran 0 siblings, 2 replies; 12+ messages in thread From: Dan Streetman @ 2016-04-28 15:36 UTC (permalink / raw) To: Minchan Kim, Nitin Gupta, Andrew Morton Cc: Sergey Senozhatsky, Seth Jennings, Yu Zhao, Linux-MM, Sergey Senozhatsky, linux-kernel, Dan Streetman, Dan Streetman Change the return type of zs_pool_stat_create() to void, and remove the logic to abort pool creation if the stat debugfs dir/file could not be created. The debugfs stat file is for debugging/information only, and doesn't affect operation of zsmalloc; there is no reason to abort creating the pool if the stat file can't be created. This was seen with zswap, which used the same name for all pool creations, which caused zsmalloc to fail to create a second pool for zswap if CONFIG_ZSMALLOC_STAT was enabled. Cc: Dan Streetman <dan.streetman@canonical.com> Signed-off-by: Dan Streetman <ddstreet@ieee.org> --- mm/zsmalloc.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c index e72efb1..25a7db2 100644 --- a/mm/zsmalloc.c +++ b/mm/zsmalloc.c @@ -567,17 +567,17 @@ static const struct file_operations zs_stat_size_ops = { .release = single_release, }; -static int zs_pool_stat_create(const char *name, struct zs_pool *pool) +static void zs_pool_stat_create(const char *name, struct zs_pool *pool) { struct dentry *entry; if (!zs_stat_root) - return -ENODEV; + return; entry = debugfs_create_dir(name, zs_stat_root); if (!entry) { pr_warn("debugfs dir <%s> creation failed\n", name); - return -ENOMEM; + return; } pool->stat_dentry = entry; @@ -586,10 +586,8 @@ static int zs_pool_stat_create(const char *name, struct zs_pool *pool) if (!entry) { pr_warn("%s: debugfs file entry <%s> creation failed\n", name, "classes"); - return -ENOMEM; + return; } - - return 0; } static void zs_pool_stat_destroy(struct zs_pool *pool) @@ -607,9 +605,8 @@ static void __exit zs_stat_exit(void) { } -static inline int zs_pool_stat_create(const char *name, struct zs_pool *pool) +static inline void zs_pool_stat_create(const char *name, struct zs_pool *pool) { - return 0; } static inline void zs_pool_stat_destroy(struct zs_pool *pool) @@ -1956,8 +1953,8 @@ struct zs_pool *zs_create_pool(const char *name, gfp_t flags) pool->flags = flags; - if (zs_pool_stat_create(name, pool)) - goto err; + /* debug only, don't abort if it fails */ + zs_pool_stat_create(name, pool); /* * Not critical, we still can use the pool -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info 2016-04-28 15:36 [PATCH] mm/zsmalloc: don't fail if can't create debugfs info Dan Streetman @ 2016-04-28 22:07 ` Andrew Morton 2016-04-29 0:38 ` Sergey Senozhatsky 2016-05-03 2:18 ` Ganesh Mahendran 1 sibling, 1 reply; 12+ messages in thread From: Andrew Morton @ 2016-04-28 22:07 UTC (permalink / raw) To: Dan Streetman Cc: Minchan Kim, Nitin Gupta, Sergey Senozhatsky, Seth Jennings, Yu Zhao, Linux-MM, Sergey Senozhatsky, linux-kernel, Dan Streetman On Thu, 28 Apr 2016 11:36:48 -0400 Dan Streetman <ddstreet@ieee.org> wrote: > Change the return type of zs_pool_stat_create() to void, and > remove the logic to abort pool creation if the stat debugfs > dir/file could not be created. > > The debugfs stat file is for debugging/information only, and doesn't > affect operation of zsmalloc; there is no reason to abort creating > the pool if the stat file can't be created. This was seen with > zswap, which used the same name for all pool creations, which caused > zsmalloc to fail to create a second pool for zswap if > CONFIG_ZSMALLOC_STAT was enabled. Needed a bit of tweaking due to http://ozlabs.org/~akpm/mmotm/broken-out/zsmalloc-reordering-function-parameter.patch From: Dan Streetman <ddstreet@ieee.org> Subject: mm/zsmalloc: don't fail if can't create debugfs info Change the return type of zs_pool_stat_create() to void, and remove the logic to abort pool creation if the stat debugfs dir/file could not be created. The debugfs stat file is for debugging/information only, and doesn't affect operation of zsmalloc; there is no reason to abort creating the pool if the stat file can't be created. This was seen with zswap, which used the same name for all pool creations, which caused zsmalloc to fail to create a second pool for zswap if CONFIG_ZSMALLOC_STAT was enabled. Signed-off-by: Dan Streetman <ddstreet@ieee.org> Cc: Dan Streetman <dan.streetman@canonical.com> Cc: Minchan Kim <minchan@kernel.org> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- mm/zsmalloc.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff -puN mm/zsmalloc.c~mm-zsmalloc-dont-fail-if-cant-create-debugfs-info mm/zsmalloc.c --- a/mm/zsmalloc.c~mm-zsmalloc-dont-fail-if-cant-create-debugfs-info +++ a/mm/zsmalloc.c @@ -568,17 +568,17 @@ static const struct file_operations zs_s .release = single_release, }; -static int zs_pool_stat_create(struct zs_pool *pool, const char *name) +static void zs_pool_stat_create(struct zs_pool *pool, const char *name) { struct dentry *entry; if (!zs_stat_root) - return -ENODEV; + return; entry = debugfs_create_dir(name, zs_stat_root); if (!entry) { pr_warn("debugfs dir <%s> creation failed\n", name); - return -ENOMEM; + return; } pool->stat_dentry = entry; @@ -587,10 +587,8 @@ static int zs_pool_stat_create(struct zs if (!entry) { pr_warn("%s: debugfs file entry <%s> creation failed\n", name, "classes"); - return -ENOMEM; + return; } - - return 0; } static void zs_pool_stat_destroy(struct zs_pool *pool) @@ -608,9 +606,8 @@ static void __exit zs_stat_exit(void) { } -static inline int zs_pool_stat_create(struct zs_pool *pool, const char *name) +static inline void zs_pool_stat_create(struct zs_pool *pool, const char *name) { - return 0; } static inline void zs_pool_stat_destroy(struct zs_pool *pool) @@ -618,7 +615,6 @@ static inline void zs_pool_stat_destroy( } #endif - /* * For each size class, zspages are divided into different groups * depending on how "full" they are. This was done so that we could @@ -1944,8 +1940,8 @@ struct zs_pool *zs_create_pool(const cha prev_class = class; } - if (zs_pool_stat_create(pool, name)) - goto err; + /* debug only, don't abort if it fails */ + zs_pool_stat_create(pool, name); /* * Not critical, we still can use the pool _ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info 2016-04-28 22:07 ` Andrew Morton @ 2016-04-29 0:38 ` Sergey Senozhatsky 2016-04-29 5:37 ` Minchan Kim 0 siblings, 1 reply; 12+ messages in thread From: Sergey Senozhatsky @ 2016-04-29 0:38 UTC (permalink / raw) To: Andrew Morton Cc: Dan Streetman, Minchan Kim, Nitin Gupta, Sergey Senozhatsky, Seth Jennings, Yu Zhao, Linux-MM, Sergey Senozhatsky, linux-kernel, Dan Streetman On (04/28/16 15:07), Andrew Morton wrote: > Needed a bit of tweaking due to > http://ozlabs.org/~akpm/mmotm/broken-out/zsmalloc-reordering-function-parameter.patch Thanks. > From: Dan Streetman <ddstreet@ieee.org> > Subject: mm/zsmalloc: don't fail if can't create debugfs info > > Change the return type of zs_pool_stat_create() to void, and > remove the logic to abort pool creation if the stat debugfs > dir/file could not be created. > > The debugfs stat file is for debugging/information only, and doesn't > affect operation of zsmalloc; there is no reason to abort creating > the pool if the stat file can't be created. This was seen with > zswap, which used the same name for all pool creations, which caused > zsmalloc to fail to create a second pool for zswap if > CONFIG_ZSMALLOC_STAT was enabled. no real objections from me. given that both zram and zswap now provide unique names for zsmalloc stats dir, this patch does not fix any "real" (observed) problem /* ENOMEM in debugfs_create_dir() is a different case */. so it's more of a cosmetic patch. FWIW, Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> -ss ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info 2016-04-29 0:38 ` Sergey Senozhatsky @ 2016-04-29 5:37 ` Minchan Kim 2016-04-29 14:50 ` Dan Streetman 0 siblings, 1 reply; 12+ messages in thread From: Minchan Kim @ 2016-04-29 5:37 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Andrew Morton, Dan Streetman, Nitin Gupta, Seth Jennings, Yu Zhao, Linux-MM, Sergey Senozhatsky, linux-kernel, Dan Streetman On Fri, Apr 29, 2016 at 09:38:24AM +0900, Sergey Senozhatsky wrote: > On (04/28/16 15:07), Andrew Morton wrote: > > Needed a bit of tweaking due to > > http://ozlabs.org/~akpm/mmotm/broken-out/zsmalloc-reordering-function-parameter.patch > > Thanks. > > > From: Dan Streetman <ddstreet@ieee.org> > > Subject: mm/zsmalloc: don't fail if can't create debugfs info > > > > Change the return type of zs_pool_stat_create() to void, and > > remove the logic to abort pool creation if the stat debugfs > > dir/file could not be created. > > > > The debugfs stat file is for debugging/information only, and doesn't > > affect operation of zsmalloc; there is no reason to abort creating > > the pool if the stat file can't be created. This was seen with > > zswap, which used the same name for all pool creations, which caused > > zsmalloc to fail to create a second pool for zswap if > > CONFIG_ZSMALLOC_STAT was enabled. > > no real objections from me. given that both zram and zswap now provide > unique names for zsmalloc stats dir, this patch does not fix any "real" > (observed) problem /* ENOMEM in debugfs_create_dir() is a different > case */. so it's more of a cosmetic patch. > Logically, I agree with Dan that debugfs is just optional so it shouldn't affect the module running *but* practically, debugfs_create_dir failure with no memory would be rare. Rather than it, we would see error from same entry naming like Dan's case. If we removes such error propagation logic in case of same naming, how do zsmalloc user can notice that debugfs entry was not created although zs_creation was successful returns success? Otherwise, future user of zsmalloc can miss it easily if they repeates same mistakes. So, what's the gain with this patch in real practice? > FWIW, > Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > > -ss ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info 2016-04-29 5:37 ` Minchan Kim @ 2016-04-29 14:50 ` Dan Streetman 2016-04-29 15:33 ` Minchan Kim 0 siblings, 1 reply; 12+ messages in thread From: Dan Streetman @ 2016-04-29 14:50 UTC (permalink / raw) To: Minchan Kim Cc: Sergey Senozhatsky, Andrew Morton, Nitin Gupta, Seth Jennings, Yu Zhao, Linux-MM, Sergey Senozhatsky, linux-kernel, Dan Streetman On Fri, Apr 29, 2016 at 1:37 AM, Minchan Kim <minchan@kernel.org> wrote: > On Fri, Apr 29, 2016 at 09:38:24AM +0900, Sergey Senozhatsky wrote: >> On (04/28/16 15:07), Andrew Morton wrote: >> > Needed a bit of tweaking due to >> > http://ozlabs.org/~akpm/mmotm/broken-out/zsmalloc-reordering-function-parameter.patch >> >> Thanks. >> >> > From: Dan Streetman <ddstreet@ieee.org> >> > Subject: mm/zsmalloc: don't fail if can't create debugfs info >> > >> > Change the return type of zs_pool_stat_create() to void, and >> > remove the logic to abort pool creation if the stat debugfs >> > dir/file could not be created. >> > >> > The debugfs stat file is for debugging/information only, and doesn't >> > affect operation of zsmalloc; there is no reason to abort creating >> > the pool if the stat file can't be created. This was seen with >> > zswap, which used the same name for all pool creations, which caused >> > zsmalloc to fail to create a second pool for zswap if >> > CONFIG_ZSMALLOC_STAT was enabled. >> >> no real objections from me. given that both zram and zswap now provide >> unique names for zsmalloc stats dir, this patch does not fix any "real" >> (observed) problem /* ENOMEM in debugfs_create_dir() is a different >> case */. so it's more of a cosmetic patch. >> > > Logically, I agree with Dan that debugfs is just optional so it > shouldn't affect the module running *but* practically, debugfs_create_dir > failure with no memory would be rare. Rather than it, we would see > error from same entry naming like Dan's case. > > If we removes such error propagation logic in case of same naming, > how do zsmalloc user can notice that debugfs entry was not created > although zs_creation was successful returns success? Does it actually matter to the caller? Since there's no way for zsmalloc to know if the stats dir/file creation failed because of EEXIST or because of ENOMEM, there's no way for it to let the caller know why it failed, either. Thus all zsmalloc can do is return a generic error, or possibly-wrong ENOMEM. In that case what will the caller do? Change the name and try again? How does the caller know what name to change it to, maybe the new name is taken too? The point of debugfs is to provide debug; failures should be ignored, because it's just debug. It should never prevent actual operation of the driver. > > Otherwise, future user of zsmalloc can miss it easily if they repeates > same mistakes. So, what's the gain with this patch in real practice? Well as far as future users, zs_create_pool doesn't document 'name' at all, and certainly doesn't clarify that 'name' should be unique across *all* zs pools that exist. And zsmalloc behavior should not be different depending on whether the ZSMALLOC_STAT param - which appears to be a debug/info only param - is enabled or not. But after any future driver using zsmalloc is created, if it did use an already-existing name - either because it was not coded to use unique names, or because of a bug that reused an existing name - which is worse? 1) driver suddenly stops working because new zs pools can't be created? 2) statistics information isn't available for some of the pools created? And, even though zswap is now patched to provide a unique name, why does zswap have to bear the burden of that? zswap doesn't care at all about the pool name, and there's no way for users to tell which zsmalloc pool corresponds to which zswap pool parameters. Future users of zsmalloc (from a code point of view, not person) probably will also not care about the zsmalloc pool name. And zsmalloc still logs the failure - so anyone looking for the stats and not finding it can easily check the logs to see the reason. The other alternative that I mentioned before, is for zsmalloc to take care of the problem itself. If the debugfs dir creation fails, it should change the name and retry; or zsmalloc can keep a list of active pool names so it knows if a new pool's name exists already or not. But neither expecting the calling code to retry with a different name, nor failing pool creation, seem like a good response when the only failure is providing debug/stats information. > > >> FWIW, >> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> >> >> -ss > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info 2016-04-29 14:50 ` Dan Streetman @ 2016-04-29 15:33 ` Minchan Kim 0 siblings, 0 replies; 12+ messages in thread From: Minchan Kim @ 2016-04-29 15:33 UTC (permalink / raw) To: Dan Streetman Cc: Minchan Kim, Sergey Senozhatsky, Andrew Morton, Nitin Gupta, Seth Jennings, Yu Zhao, Linux-MM, Sergey Senozhatsky, linux-kernel, Dan Streetman On Fri, Apr 29, 2016 at 10:50:13AM -0400, Dan Streetman wrote: > On Fri, Apr 29, 2016 at 1:37 AM, Minchan Kim <minchan@kernel.org> wrote: > > On Fri, Apr 29, 2016 at 09:38:24AM +0900, Sergey Senozhatsky wrote: > >> On (04/28/16 15:07), Andrew Morton wrote: > >> > Needed a bit of tweaking due to > >> > http://ozlabs.org/~akpm/mmotm/broken-out/zsmalloc-reordering-function-parameter.patch > >> > >> Thanks. > >> > >> > From: Dan Streetman <ddstreet@ieee.org> > >> > Subject: mm/zsmalloc: don't fail if can't create debugfs info > >> > > >> > Change the return type of zs_pool_stat_create() to void, and > >> > remove the logic to abort pool creation if the stat debugfs > >> > dir/file could not be created. > >> > > >> > The debugfs stat file is for debugging/information only, and doesn't > >> > affect operation of zsmalloc; there is no reason to abort creating > >> > the pool if the stat file can't be created. This was seen with > >> > zswap, which used the same name for all pool creations, which caused > >> > zsmalloc to fail to create a second pool for zswap if > >> > CONFIG_ZSMALLOC_STAT was enabled. > >> > >> no real objections from me. given that both zram and zswap now provide > >> unique names for zsmalloc stats dir, this patch does not fix any "real" > >> (observed) problem /* ENOMEM in debugfs_create_dir() is a different > >> case */. so it's more of a cosmetic patch. > >> > > > > Logically, I agree with Dan that debugfs is just optional so it > > shouldn't affect the module running *but* practically, debugfs_create_dir > > failure with no memory would be rare. Rather than it, we would see > > error from same entry naming like Dan's case. > > > > If we removes such error propagation logic in case of same naming, > > how do zsmalloc user can notice that debugfs entry was not created > > although zs_creation was successful returns success? > > Does it actually matter to the caller? > > Since there's no way for zsmalloc to know if the stats dir/file > creation failed because of EEXIST or because of ENOMEM, there's no way > for it to let the caller know why it failed, either. Thus all > zsmalloc can do is return a generic error, or possibly-wrong ENOMEM. > In that case what will the caller do? Change the name and try again? > How does the caller know what name to change it to, maybe the new name > is taken too? > > The point of debugfs is to provide debug; failures should be ignored, > because it's just debug. It should never prevent actual operation of > the driver. > > > > > Otherwise, future user of zsmalloc can miss it easily if they repeates > > same mistakes. So, what's the gain with this patch in real practice? > > Well as far as future users, zs_create_pool doesn't document 'name' at > all, and certainly doesn't clarify that 'name' should be unique across > *all* zs pools that exist. And zsmalloc behavior should not be > different depending on whether the ZSMALLOC_STAT param - which appears > to be a debug/info only param - is enabled or not. Fair enough. Then, could you apply it to zs_stat_init? We could make zs_stat_init to return void to not affect module loading, too. As well, we should be okay in zs_stat_root failue, too. I hope your patch handles them all in this chance. Thanks for the looking this. > > But after any future driver using zsmalloc is created, if it did use > an already-existing name - either because it was not coded to use > unique names, or because of a bug that reused an existing name - which > is worse? > 1) driver suddenly stops working because new zs pools can't be created? > 2) statistics information isn't available for some of the pools created? > > And, even though zswap is now patched to provide a unique name, why > does zswap have to bear the burden of that? zswap doesn't care at all > about the pool name, and there's no way for users to tell which > zsmalloc pool corresponds to which zswap pool parameters. Future > users of zsmalloc (from a code point of view, not person) probably > will also not care about the zsmalloc pool name. And zsmalloc still > logs the failure - so anyone looking for the stats and not finding it > can easily check the logs to see the reason. > > The other alternative that I mentioned before, is for zsmalloc to take > care of the problem itself. If the debugfs dir creation fails, it > should change the name and retry; or zsmalloc can keep a list of > active pool names so it knows if a new pool's name exists already or > not. But neither expecting the calling code to retry with a different > name, nor failing pool creation, seem like a good response when the > only failure is providing debug/stats information. > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info 2016-04-28 15:36 [PATCH] mm/zsmalloc: don't fail if can't create debugfs info Dan Streetman 2016-04-28 22:07 ` Andrew Morton @ 2016-05-03 2:18 ` Ganesh Mahendran 2016-05-19 15:18 ` [PATCHv2] " Dan Streetman 1 sibling, 1 reply; 12+ messages in thread From: Ganesh Mahendran @ 2016-05-03 2:18 UTC (permalink / raw) To: Dan Streetman Cc: Minchan Kim, Nitin Gupta, Andrew Morton, Sergey Senozhatsky, Seth Jennings, Yu Zhao, Linux-MM, Sergey Senozhatsky, linux-kernel, Dan Streetman Hello, Dan: 2016-04-28 23:36 GMT+08:00 Dan Streetman <ddstreet@ieee.org>: > Change the return type of zs_pool_stat_create() to void, and > remove the logic to abort pool creation if the stat debugfs > dir/file could not be created. > > The debugfs stat file is for debugging/information only, and doesn't > affect operation of zsmalloc; there is no reason to abort creating > the pool if the stat file can't be created. This was seen with > zswap, which used the same name for all pool creations, which caused > zsmalloc to fail to create a second pool for zswap if > CONFIG_ZSMALLOC_STAT was enabled. > > Cc: Dan Streetman <dan.streetman@canonical.com> > Signed-off-by: Dan Streetman <ddstreet@ieee.org> > --- > mm/zsmalloc.c | 17 +++++++---------- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c > index e72efb1..25a7db2 100644 > --- a/mm/zsmalloc.c > +++ b/mm/zsmalloc.c > @@ -567,17 +567,17 @@ static const struct file_operations zs_stat_size_ops = { > .release = single_release, > }; > > -static int zs_pool_stat_create(const char *name, struct zs_pool *pool) > +static void zs_pool_stat_create(const char *name, struct zs_pool *pool) > { > struct dentry *entry; > > if (!zs_stat_root) > - return -ENODEV; > + return; Since the error will not be propagated, Would it be better if you add some pr_warn information here(also in zs_stat_init() if you send your V2 patch as Minchan suggested)? It will be useful for developers to know the reason of failed to create debugfs file/dir. Thanks. > > entry = debugfs_create_dir(name, zs_stat_root); > if (!entry) { > pr_warn("debugfs dir <%s> creation failed\n", name); > - return -ENOMEM; > + return; > } > pool->stat_dentry = entry; > > @@ -586,10 +586,8 @@ static int zs_pool_stat_create(const char *name, struct zs_pool *pool) > if (!entry) { > pr_warn("%s: debugfs file entry <%s> creation failed\n", > name, "classes"); > - return -ENOMEM; > + return; > } > - > - return 0; > } > > static void zs_pool_stat_destroy(struct zs_pool *pool) > @@ -607,9 +605,8 @@ static void __exit zs_stat_exit(void) > { > } > > -static inline int zs_pool_stat_create(const char *name, struct zs_pool *pool) > +static inline void zs_pool_stat_create(const char *name, struct zs_pool *pool) > { > - return 0; > } > > static inline void zs_pool_stat_destroy(struct zs_pool *pool) > @@ -1956,8 +1953,8 @@ struct zs_pool *zs_create_pool(const char *name, gfp_t flags) > > pool->flags = flags; > > - if (zs_pool_stat_create(name, pool)) > - goto err; > + /* debug only, don't abort if it fails */ > + zs_pool_stat_create(name, pool); > > /* > * Not critical, we still can use the pool > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCHv2] mm/zsmalloc: don't fail if can't create debugfs info 2016-05-03 2:18 ` Ganesh Mahendran @ 2016-05-19 15:18 ` Dan Streetman 2016-05-20 2:33 ` Ganesh Mahendran ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Dan Streetman @ 2016-05-19 15:18 UTC (permalink / raw) To: Minchan Kim Cc: Nitin Gupta, Ganesh Mahendran, Andrew Morton, Sergey Senozhatsky, Seth Jennings, Yu Zhao, Linux-MM, Sergey Senozhatsky, linux-kernel, Dan Streetman, Dan Streetman Change the return type of zs_pool_stat_create() to void, and remove the logic to abort pool creation if the stat debugfs dir/file could not be created. The debugfs stat file is for debugging/information only, and doesn't affect operation of zsmalloc; there is no reason to abort creating the pool if the stat file can't be created. This was seen with zswap, which used the same name for all pool creations, which caused zsmalloc to fail to create a second pool for zswap if CONFIG_ZSMALLOC_STAT was enabled. Signed-off-by: Dan Streetman <ddstreet@ieee.org> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> Cc: Dan Streetman <dan.streetman@canonical.com> Cc: Minchan Kim <minchan@kernel.org> --- Changes since v1: -add pr_warn to all stat failure cases -do not prevent module loading on stat failure mm/zsmalloc.c | 51 ++++++++++++++++++++++----------------------------- 1 file changed, 22 insertions(+), 29 deletions(-) diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c index aba39a2..b6d4f25 100644 --- a/mm/zsmalloc.c +++ b/mm/zsmalloc.c @@ -45,6 +45,8 @@ * */ +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + #include <linux/module.h> #include <linux/kernel.h> #include <linux/sched.h> @@ -483,16 +485,16 @@ static inline unsigned long zs_stat_get(struct size_class *class, #ifdef CONFIG_ZSMALLOC_STAT -static int __init zs_stat_init(void) +static void __init zs_stat_init(void) { - if (!debugfs_initialized()) - return -ENODEV; + if (!debugfs_initialized()) { + pr_warn("debugfs not available, stat dir not created\n"); + return; + } zs_stat_root = debugfs_create_dir("zsmalloc", NULL); if (!zs_stat_root) - return -ENOMEM; - - return 0; + pr_warn("debugfs 'zsmalloc' stat dir creation failed\n"); } static void __exit zs_stat_exit(void) @@ -573,17 +575,19 @@ static const struct file_operations zs_stat_size_ops = { .release = single_release, }; -static int zs_pool_stat_create(struct zs_pool *pool, const char *name) +static void zs_pool_stat_create(struct zs_pool *pool, const char *name) { struct dentry *entry; - if (!zs_stat_root) - return -ENODEV; + if (!zs_stat_root) { + pr_warn("no root stat dir, not creating <%s> stat dir\n", name); + return; + } entry = debugfs_create_dir(name, zs_stat_root); if (!entry) { pr_warn("debugfs dir <%s> creation failed\n", name); - return -ENOMEM; + return; } pool->stat_dentry = entry; @@ -592,10 +596,9 @@ static int zs_pool_stat_create(struct zs_pool *pool, const char *name) if (!entry) { pr_warn("%s: debugfs file entry <%s> creation failed\n", name, "classes"); - return -ENOMEM; + debugfs_remove_recursive(pool->stat_dentry); + pool->stat_dentry = NULL; } - - return 0; } static void zs_pool_stat_destroy(struct zs_pool *pool) @@ -604,18 +607,16 @@ static void zs_pool_stat_destroy(struct zs_pool *pool) } #else /* CONFIG_ZSMALLOC_STAT */ -static int __init zs_stat_init(void) +static void __init zs_stat_init(void) { - return 0; } static void __exit zs_stat_exit(void) { } -static inline int zs_pool_stat_create(struct zs_pool *pool, const char *name) +static inline void zs_pool_stat_create(struct zs_pool *pool, const char *name) { - return 0; } static inline void zs_pool_stat_destroy(struct zs_pool *pool) @@ -623,7 +624,6 @@ static inline void zs_pool_stat_destroy(struct zs_pool *pool) } #endif - /* * For each size class, zspages are divided into different groups * depending on how "full" they are. This was done so that we could @@ -1952,8 +1952,8 @@ struct zs_pool *zs_create_pool(const char *name) prev_class = class; } - if (zs_pool_stat_create(pool, name)) - goto err; + /* debug only, don't abort if it fails */ + zs_pool_stat_create(pool, name); /* * Not critical, we still can use the pool @@ -2015,17 +2015,10 @@ static int __init zs_init(void) zpool_register_driver(&zs_zpool_driver); #endif - ret = zs_stat_init(); - if (ret) { - pr_err("zs stat initialization failed\n"); - goto stat_fail; - } + zs_stat_init(); + return 0; -stat_fail: -#ifdef CONFIG_ZPOOL - zpool_unregister_driver(&zs_zpool_driver); -#endif notifier_fail: zs_unregister_cpu_notifier(); -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCHv2] mm/zsmalloc: don't fail if can't create debugfs info 2016-05-19 15:18 ` [PATCHv2] " Dan Streetman @ 2016-05-20 2:33 ` Ganesh Mahendran 2016-05-20 4:08 ` Sergey Senozhatsky 2016-05-23 3:03 ` Minchan Kim 2 siblings, 0 replies; 12+ messages in thread From: Ganesh Mahendran @ 2016-05-20 2:33 UTC (permalink / raw) To: Dan Streetman Cc: Minchan Kim, Nitin Gupta, Andrew Morton, Sergey Senozhatsky, Seth Jennings, Yu Zhao, Linux-MM, Sergey Senozhatsky, linux-kernel, Dan Streetman 2016-05-19 23:18 GMT+08:00 Dan Streetman <ddstreet@ieee.org>: > Change the return type of zs_pool_stat_create() to void, and > remove the logic to abort pool creation if the stat debugfs > dir/file could not be created. > > The debugfs stat file is for debugging/information only, and doesn't > affect operation of zsmalloc; there is no reason to abort creating > the pool if the stat file can't be created. This was seen with > zswap, which used the same name for all pool creations, which caused > zsmalloc to fail to create a second pool for zswap if > CONFIG_ZSMALLOC_STAT was enabled. > > Signed-off-by: Dan Streetman <ddstreet@ieee.org> > Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > Cc: Dan Streetman <dan.streetman@canonical.com> > Cc: Minchan Kim <minchan@kernel.org> > > --- > Changes since v1: > -add pr_warn to all stat failure cases > -do not prevent module loading on stat failure > Reviewed-by: Ganesh Mahendran <opensource.ganesh@gmail.com> > mm/zsmalloc.c | 51 ++++++++++++++++++++++----------------------------- > 1 file changed, 22 insertions(+), 29 deletions(-) > > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c > index aba39a2..b6d4f25 100644 > --- a/mm/zsmalloc.c > +++ b/mm/zsmalloc.c > @@ -45,6 +45,8 @@ > * > */ > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > #include <linux/module.h> > #include <linux/kernel.h> > #include <linux/sched.h> > @@ -483,16 +485,16 @@ static inline unsigned long zs_stat_get(struct size_class *class, > > #ifdef CONFIG_ZSMALLOC_STAT > > -static int __init zs_stat_init(void) > +static void __init zs_stat_init(void) > { > - if (!debugfs_initialized()) > - return -ENODEV; > + if (!debugfs_initialized()) { > + pr_warn("debugfs not available, stat dir not created\n"); > + return; > + } > > zs_stat_root = debugfs_create_dir("zsmalloc", NULL); > if (!zs_stat_root) > - return -ENOMEM; > - > - return 0; > + pr_warn("debugfs 'zsmalloc' stat dir creation failed\n"); > } > > static void __exit zs_stat_exit(void) > @@ -573,17 +575,19 @@ static const struct file_operations zs_stat_size_ops = { > .release = single_release, > }; > > -static int zs_pool_stat_create(struct zs_pool *pool, const char *name) > +static void zs_pool_stat_create(struct zs_pool *pool, const char *name) > { > struct dentry *entry; > > - if (!zs_stat_root) > - return -ENODEV; > + if (!zs_stat_root) { > + pr_warn("no root stat dir, not creating <%s> stat dir\n", name); > + return; > + } > > entry = debugfs_create_dir(name, zs_stat_root); > if (!entry) { > pr_warn("debugfs dir <%s> creation failed\n", name); > - return -ENOMEM; > + return; > } > pool->stat_dentry = entry; > > @@ -592,10 +596,9 @@ static int zs_pool_stat_create(struct zs_pool *pool, const char *name) > if (!entry) { > pr_warn("%s: debugfs file entry <%s> creation failed\n", > name, "classes"); > - return -ENOMEM; > + debugfs_remove_recursive(pool->stat_dentry); > + pool->stat_dentry = NULL; > } > - > - return 0; > } > > static void zs_pool_stat_destroy(struct zs_pool *pool) > @@ -604,18 +607,16 @@ static void zs_pool_stat_destroy(struct zs_pool *pool) > } > > #else /* CONFIG_ZSMALLOC_STAT */ > -static int __init zs_stat_init(void) > +static void __init zs_stat_init(void) > { > - return 0; > } > > static void __exit zs_stat_exit(void) > { > } > > -static inline int zs_pool_stat_create(struct zs_pool *pool, const char *name) > +static inline void zs_pool_stat_create(struct zs_pool *pool, const char *name) > { > - return 0; > } > > static inline void zs_pool_stat_destroy(struct zs_pool *pool) > @@ -623,7 +624,6 @@ static inline void zs_pool_stat_destroy(struct zs_pool *pool) > } > #endif > > - > /* > * For each size class, zspages are divided into different groups > * depending on how "full" they are. This was done so that we could > @@ -1952,8 +1952,8 @@ struct zs_pool *zs_create_pool(const char *name) > prev_class = class; > } > > - if (zs_pool_stat_create(pool, name)) > - goto err; > + /* debug only, don't abort if it fails */ > + zs_pool_stat_create(pool, name); > > /* > * Not critical, we still can use the pool > @@ -2015,17 +2015,10 @@ static int __init zs_init(void) > zpool_register_driver(&zs_zpool_driver); > #endif > > - ret = zs_stat_init(); > - if (ret) { > - pr_err("zs stat initialization failed\n"); > - goto stat_fail; > - } > + zs_stat_init(); > + > return 0; > > -stat_fail: > -#ifdef CONFIG_ZPOOL > - zpool_unregister_driver(&zs_zpool_driver); > -#endif > notifier_fail: > zs_unregister_cpu_notifier(); > > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2] mm/zsmalloc: don't fail if can't create debugfs info 2016-05-19 15:18 ` [PATCHv2] " Dan Streetman 2016-05-20 2:33 ` Ganesh Mahendran @ 2016-05-20 4:08 ` Sergey Senozhatsky 2016-05-20 10:32 ` Dan Streetman 2016-05-23 3:03 ` Minchan Kim 2 siblings, 1 reply; 12+ messages in thread From: Sergey Senozhatsky @ 2016-05-20 4:08 UTC (permalink / raw) To: Dan Streetman Cc: Minchan Kim, Nitin Gupta, Ganesh Mahendran, Andrew Morton, Sergey Senozhatsky, Seth Jennings, Yu Zhao, Linux-MM, Sergey Senozhatsky, linux-kernel, Dan Streetman On (05/19/16 11:18), Dan Streetman wrote: [..] > zs_stat_root = debugfs_create_dir("zsmalloc", NULL); > if (!zs_stat_root) > - return -ENOMEM; > - > - return 0; > + pr_warn("debugfs 'zsmalloc' stat dir creation failed\n"); > } > > static void __exit zs_stat_exit(void) > @@ -573,17 +575,19 @@ static const struct file_operations zs_stat_size_ops = { > .release = single_release, > }; > > -static int zs_pool_stat_create(struct zs_pool *pool, const char *name) > +static void zs_pool_stat_create(struct zs_pool *pool, const char *name) > { > struct dentry *entry; > > - if (!zs_stat_root) > - return -ENODEV; > + if (!zs_stat_root) { > + pr_warn("no root stat dir, not creating <%s> stat dir\n", name); > + return; > + } just a small nit, there are basically two warn messages now for `!zs_stat_root': debugfs 'zsmalloc' stat dir creation failed no root stat dir, not creating <%s> stat dir may be we need only one of them; but no strong opinions. -ss ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2] mm/zsmalloc: don't fail if can't create debugfs info 2016-05-20 4:08 ` Sergey Senozhatsky @ 2016-05-20 10:32 ` Dan Streetman 0 siblings, 0 replies; 12+ messages in thread From: Dan Streetman @ 2016-05-20 10:32 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Minchan Kim, Nitin Gupta, Ganesh Mahendran, Andrew Morton, Seth Jennings, Yu Zhao, Linux-MM, Sergey Senozhatsky, linux-kernel, Dan Streetman On Fri, May 20, 2016 at 12:08 AM, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > On (05/19/16 11:18), Dan Streetman wrote: > [..] >> zs_stat_root = debugfs_create_dir("zsmalloc", NULL); >> if (!zs_stat_root) >> - return -ENOMEM; >> - >> - return 0; >> + pr_warn("debugfs 'zsmalloc' stat dir creation failed\n"); >> } >> >> static void __exit zs_stat_exit(void) >> @@ -573,17 +575,19 @@ static const struct file_operations zs_stat_size_ops = { >> .release = single_release, >> }; >> >> -static int zs_pool_stat_create(struct zs_pool *pool, const char *name) >> +static void zs_pool_stat_create(struct zs_pool *pool, const char *name) >> { >> struct dentry *entry; >> >> - if (!zs_stat_root) >> - return -ENODEV; >> + if (!zs_stat_root) { >> + pr_warn("no root stat dir, not creating <%s> stat dir\n", name); >> + return; >> + } > > just a small nit, there are basically two warn messages now for > `!zs_stat_root': > > debugfs 'zsmalloc' stat dir creation failed > no root stat dir, not creating <%s> stat dir They're logged at different times though, the first at module load time, the second at every pool creation time. So while they may be logged together if the module is loaded because a pool is being created, any later pools created will only log the second message. > > may be we need only one of them; but no strong opinions. If we drop either, I'd drop the first, but I think it could be useful also in case zsmalloc is built-in or manually loaded without creating a pool. > > -ss ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2] mm/zsmalloc: don't fail if can't create debugfs info 2016-05-19 15:18 ` [PATCHv2] " Dan Streetman 2016-05-20 2:33 ` Ganesh Mahendran 2016-05-20 4:08 ` Sergey Senozhatsky @ 2016-05-23 3:03 ` Minchan Kim 2 siblings, 0 replies; 12+ messages in thread From: Minchan Kim @ 2016-05-23 3:03 UTC (permalink / raw) To: Dan Streetman Cc: Nitin Gupta, Ganesh Mahendran, Andrew Morton, Sergey Senozhatsky, Seth Jennings, Yu Zhao, Linux-MM, Sergey Senozhatsky, linux-kernel, Dan Streetman On Thu, May 19, 2016 at 11:18:43AM -0400, Dan Streetman wrote: > Change the return type of zs_pool_stat_create() to void, and > remove the logic to abort pool creation if the stat debugfs > dir/file could not be created. > > The debugfs stat file is for debugging/information only, and doesn't > affect operation of zsmalloc; there is no reason to abort creating > the pool if the stat file can't be created. This was seen with > zswap, which used the same name for all pool creations, which caused > zsmalloc to fail to create a second pool for zswap if > CONFIG_ZSMALLOC_STAT was enabled. > > Signed-off-by: Dan Streetman <ddstreet@ieee.org> > Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > Cc: Dan Streetman <dan.streetman@canonical.com> > Cc: Minchan Kim <minchan@kernel.org> Acked-by: Minchan Kim <minchan@kernel.org> However, Andrew already sent old version to upstream. Andrew, Could you send revert patch of [1] in linus's tree and send this instead of it if you have chance? [1] d34f615720d1 mm/zsmalloc: don't fail if can't create debugfs info Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-05-23 3:03 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-28 15:36 [PATCH] mm/zsmalloc: don't fail if can't create debugfs info Dan Streetman 2016-04-28 22:07 ` Andrew Morton 2016-04-29 0:38 ` Sergey Senozhatsky 2016-04-29 5:37 ` Minchan Kim 2016-04-29 14:50 ` Dan Streetman 2016-04-29 15:33 ` Minchan Kim 2016-05-03 2:18 ` Ganesh Mahendran 2016-05-19 15:18 ` [PATCHv2] " Dan Streetman 2016-05-20 2:33 ` Ganesh Mahendran 2016-05-20 4:08 ` Sergey Senozhatsky 2016-05-20 10:32 ` Dan Streetman 2016-05-23 3:03 ` Minchan Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).