Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1390331 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm/zsmalloc: don't fail if can't create debugfs info Dan Streetman <ddstreet@ieee.org> - 2016-04-28 17:40 +0200
    Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info Andrew Morton <akpm@linux-foundation.org> - 2016-04-29 00:10 +0200
      Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-04-29 02:40 +0200
        Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info Minchan Kim <minchan@kernel.org> - 2016-04-29 07:40 +0200
          Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info Dan Streetman <ddstreet@ieee.org> - 2016-04-29 17:00 +0200
            Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info Minchan Kim <minchan@kernel.org> - 2016-04-29 17:40 +0200
    Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info Ganesh Mahendran <opensource.ganesh@gmail.com> - 2016-05-03 04:20 +0200

#1390331 — [PATCH] 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.

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

[toc] | [next] | [standalone]

#1390580

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
_

[toc] | [prev] | [next] | [standalone]

#1390671

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

[toc] | [prev] | [next] | [standalone]

#1390767

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

[toc] | [prev] | [next] | [standalone]

#1391207

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>

[toc] | [prev] | [next] | [standalone]

#1391261

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

[toc] | [prev] | [next] | [standalone]

#1392990

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
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web