[PATCH] mm/zsmalloc: don't fail if can't create debugfs info

[PATCH] mm/zsmalloc: don't fail if can't create debugfs info

[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

--
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>

Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info

[Andrew Morton]

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
_

--
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>

Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info

[Sergey Senozhatsky]

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

--
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>

Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info

[Minchan Kim]

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

--
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>

Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info

[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>

--
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>

Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info

[Minchan Kim]

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.
> 

--
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>

Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info

[Ganesh Mahendran]

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
>

--
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>

[PATCHv2] mm/zsmalloc: don't fail if can't create debugfs info

[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

--
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>

Re: [PATCHv2] mm/zsmalloc: don't fail if can't create debugfs info

[Ganesh Mahendran]

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
>

--
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>

Re: [PATCHv2] mm/zsmalloc: don't fail if can't create debugfs info

[Sergey Senozhatsky]

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

--
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>

Re: [PATCHv2] mm/zsmalloc: don't fail if can't create debugfs info

[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

--
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>

Re: [PATCHv2] mm/zsmalloc: don't fail if can't create debugfs info

[Minchan Kim]

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.

--
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>