[PATCH] mm/zsmalloc: simplify shrinker init/destroy

[PATCH] mm/zsmalloc: simplify shrinker init/destroy

[Aliaksei Karaliou]

Structure zs_pool has special flag to indicate success of shrinker
initialization. unregister_shrinker() has improved and can detect
by itself whether actual deinitialization should be performed or not,
so extra flag becomes redundant.

Signed-off-by: Aliaksei Karaliou <akaraliou.dev@gmail.com>
---
 mm/zsmalloc.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 685049a..6b6aaa9 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -256,11 +256,7 @@ struct zs_pool {
 
 	/* Compact classes */
 	struct shrinker shrinker;
-	/*
-	 * To signify that register_shrinker() was successful
-	 * and unregister_shrinker() will not Oops.
-	 */
-	bool shrinker_enabled;
+
 #ifdef CONFIG_ZSMALLOC_STAT
 	struct dentry *stat_dentry;
 #endif
@@ -2323,10 +2319,7 @@ static unsigned long zs_shrinker_count(struct shrinker *shrinker,
 
 static void zs_unregister_shrinker(struct zs_pool *pool)
 {
-	if (pool->shrinker_enabled) {
-		unregister_shrinker(&pool->shrinker);
-		pool->shrinker_enabled = false;
-	}
+	unregister_shrinker(&pool->shrinker);
 }
 
 static int zs_register_shrinker(struct zs_pool *pool)
@@ -2428,8 +2421,8 @@ struct zs_pool *zs_create_pool(const char *name)
 	 * Not critical, we still can use the pool
 	 * and user can trigger compaction manually.
 	 */
-	if (zs_register_shrinker(pool) == 0)
-		pool->shrinker_enabled = true;
+	(void) zs_register_shrinker(pool);
+
 	return pool;
 
 err:
-- 
2.1.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: simplify shrinker init/destroy

[Sergey Senozhatsky]

Hi,

On (12/19/17 12:21), Aliaksei Karaliou wrote:
> unregister_shrinker() has improved and can detect by itself whether
> actual deinitialization should be performed or not, so extra flag
> becomes redundant.

yay... could have happened 2 years earlier
https://marc.info/?l=linux-mm&m=143658322724908&w=2

> Signed-off-by: Aliaksei Karaliou <akaraliou.dev@gmail.com>

could you add <linux/shrinker.h> include and re-spin the patch?

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

[PATCH v2] mm/zsmalloc: simplify shrinker init/destroy

[Aliaksei Karaliou]

Structure zs_pool has special flag to indicate success of shrinker
initialization. unregister_shrinker() has improved and can detect
by itself whether actual deinitialization should be performed or not,
so extra flag becomes redundant.

Signed-off-by: Aliaksei Karaliou <akaraliou.dev@gmail.com>
---
 mm/zsmalloc.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

v2: Added include <linux/shrinker.h> as suggested by Sergey Senozhatsky.

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 685049a..628a1bc 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -53,6 +53,7 @@
 #include <linux/mount.h>
 #include <linux/migrate.h>
 #include <linux/pagemap.h>
+#include <linux/shrinker.h>
 
 #define ZSPAGE_MAGIC	0x58
 
@@ -256,11 +257,7 @@ struct zs_pool {
 
 	/* Compact classes */
 	struct shrinker shrinker;
-	/*
-	 * To signify that register_shrinker() was successful
-	 * and unregister_shrinker() will not Oops.
-	 */
-	bool shrinker_enabled;
+
 #ifdef CONFIG_ZSMALLOC_STAT
 	struct dentry *stat_dentry;
 #endif
@@ -2323,10 +2320,7 @@ static unsigned long zs_shrinker_count(struct shrinker *shrinker,
 
 static void zs_unregister_shrinker(struct zs_pool *pool)
 {
-	if (pool->shrinker_enabled) {
-		unregister_shrinker(&pool->shrinker);
-		pool->shrinker_enabled = false;
-	}
+	unregister_shrinker(&pool->shrinker);
 }
 
 static int zs_register_shrinker(struct zs_pool *pool)
@@ -2428,8 +2422,8 @@ struct zs_pool *zs_create_pool(const char *name)
 	 * Not critical, we still can use the pool
 	 * and user can trigger compaction manually.
 	 */
-	if (zs_register_shrinker(pool) == 0)
-		pool->shrinker_enabled = true;
+	(void) zs_register_shrinker(pool);
+
 	return pool;
 
 err:
-- 
2.1.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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Sergey Senozhatsky]

Ccing Andrew
1513680552-9798-1-git-send-email-akaraliou.dev@gmail.com

On (12/19/17 13:49), Aliaksei Karaliou wrote:
> 
> Structure zs_pool has special flag to indicate success of shrinker
> initialization. unregister_shrinker() has improved and can detect
> by itself whether actual deinitialization should be performed or not,
> so extra flag becomes redundant.
> 
> Signed-off-by: Aliaksei Karaliou <akaraliou.dev@gmail.com>

looks good to me.

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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Sergey Senozhatsky]

d'oh... actually Ccing Andrew.

	-ss

---

Ccing Andrew
1513680552-9798-1-git-send-email-akaraliou.dev@gmail.com

On (12/19/17 13:49), Aliaksei Karaliou wrote:
> 
> Structure zs_pool has special flag to indicate success of shrinker
> initialization. unregister_shrinker() has improved and can detect
> by itself whether actual deinitialization should be performed or not,
> so extra flag becomes redundant.
> 
> Signed-off-by: Aliaksei Karaliou <akaraliou.dev@gmail.com>

looks good to me.

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

--
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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Michal Hocko]

On Tue 19-12-17 13:49:12, Aliaksei Karaliou wrote:
[...]
> @@ -2428,8 +2422,8 @@ struct zs_pool *zs_create_pool(const char *name)
>  	 * Not critical, we still can use the pool
>  	 * and user can trigger compaction manually.
>  	 */
> -	if (zs_register_shrinker(pool) == 0)
> -		pool->shrinker_enabled = true;
> +	(void) zs_register_shrinker(pool);
> +
>  	return pool;

So what will happen if the pool is alive and used without any shrinker?
How do objects get freed?
-- 
Michal Hocko
SUSE Labs

--
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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Sergey Senozhatsky]

Hi Michal,

On (12/19/17 16:13), Michal Hocko wrote:
> On Tue 19-12-17 13:49:12, Aliaksei Karaliou wrote:
> [...]
> > @@ -2428,8 +2422,8 @@ struct zs_pool *zs_create_pool(const char *name)
> >  	 * Not critical, we still can use the pool
> >  	 * and user can trigger compaction manually.
> >  	 */
> > -	if (zs_register_shrinker(pool) == 0)
> > -		pool->shrinker_enabled = true;
> > +	(void) zs_register_shrinker(pool);
> > +
> >  	return pool;
> 
> So what will happen if the pool is alive and used without any shrinker?
> How do objects get freed?

we use shrinker for "optional" de-fragmentation of zsmalloc pools. we
don't free any objects from that path. just move them around within their
size classes - to consolidate objects and to, may be, free unused pages
[but we first need to make them "unused"]. it's not a mandatory thing for
zsmalloc, we are just trying to be nice.

	-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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Aliaksei Karaliou]

On 12/19/2017 06:25 PM, Sergey Senozhatsky wrote:

> Hi Michal,
>
> On (12/19/17 16:13), Michal Hocko wrote:
>> On Tue 19-12-17 13:49:12, Aliaksei Karaliou wrote:
>> [...]
>>> @@ -2428,8 +2422,8 @@ struct zs_pool *zs_create_pool(const char *name)
>>>  	 * Not critical, we still can use the pool
>>>  	 * and user can trigger compaction manually.
>>>  	 */
>>> -	if (zs_register_shrinker(pool) == 0)
>>> -		pool->shrinker_enabled = true;
>>> +	(void) zs_register_shrinker(pool);
>>> +
>>>  	return pool;
>> So what will happen if the pool is alive and used without any shrinker?
>> How do objects get freed?
> we use shrinker for "optional" de-fragmentation of zsmalloc pools. we
> don't free any objects from that path. just move them around within their
> size classes - to consolidate objects and to, may be, free unused pages
> [but we first need to make them "unused"]. it's not a mandatory thing for
> zsmalloc, we are just trying to be nice.
>
> 	-ss

Thanks a lot for clarification from your side, Sergey.

Best regards,
    Aliaksei.

--
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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Michal Hocko]

On Wed 20-12-17 00:25:36, Sergey Senozhatsky wrote:
> Hi Michal,
> 
> On (12/19/17 16:13), Michal Hocko wrote:
> > On Tue 19-12-17 13:49:12, Aliaksei Karaliou wrote:
> > [...]
> > > @@ -2428,8 +2422,8 @@ struct zs_pool *zs_create_pool(const char *name)
> > >  	 * Not critical, we still can use the pool
> > >  	 * and user can trigger compaction manually.
> > >  	 */
> > > -	if (zs_register_shrinker(pool) == 0)
> > > -		pool->shrinker_enabled = true;
> > > +	(void) zs_register_shrinker(pool);
> > > +
> > >  	return pool;
> > 
> > So what will happen if the pool is alive and used without any shrinker?
> > How do objects get freed?
> 
> we use shrinker for "optional" de-fragmentation of zsmalloc pools. we
> don't free any objects from that path. just move them around within their
> size classes - to consolidate objects and to, may be, free unused pages
> [but we first need to make them "unused"]. it's not a mandatory thing for
> zsmalloc, we are just trying to be nice.

OK, it smells like an abuse of the API but please add a comment
clarifying that.

Thanks!
-- 
Michal Hocko
SUSE Labs

--
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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Aliaksei Karaliou]

On 12/19/2017 06:58 PM, Michal Hocko wrote:
> On Wed 20-12-17 00:25:36, Sergey Senozhatsky wrote:
>> Hi Michal,
>>
>> On (12/19/17 16:13), Michal Hocko wrote:
>>> On Tue 19-12-17 13:49:12, Aliaksei Karaliou wrote:
>>> [...]
>>>> @@ -2428,8 +2422,8 @@ struct zs_pool *zs_create_pool(const char *name)
>>>>   	 * Not critical, we still can use the pool
>>>>   	 * and user can trigger compaction manually.
>>>>   	 */
>>>> -	if (zs_register_shrinker(pool) == 0)
>>>> -		pool->shrinker_enabled = true;
>>>> +	(void) zs_register_shrinker(pool);
>>>> +
>>>>   	return pool;
>>> So what will happen if the pool is alive and used without any shrinker?
>>> How do objects get freed?
>> we use shrinker for "optional" de-fragmentation of zsmalloc pools. we
>> don't free any objects from that path. just move them around within their
>> size classes - to consolidate objects and to, may be, free unused pages
>> [but we first need to make them "unused"]. it's not a mandatory thing for
>> zsmalloc, we are just trying to be nice.
> OK, it smells like an abuse of the API but please add a comment
> clarifying that.
>
> Thanks!
I can update the existing comment to be like that:
         /*
          * Not critical since shrinker is only used to trigger internal
          * de-fragmentation of the pool which is pretty optional thing.
          * If registration fails we still can use the pool normally and
          * user can trigger compaction manually. Thus, ignore return code.
          */

Sergey, does this sound well to you ? Or not clear enough, Michal ?

Best regards,
     Aliaksei

--
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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Andrew Morton]

On Tue, 19 Dec 2017 20:45:17 +0300 Aliaksei Karaliou <akaraliou.dev@gmail.com> wrote:

> >>> So what will happen if the pool is alive and used without any shrinker?
> >>> How do objects get freed?
> >> we use shrinker for "optional" de-fragmentation of zsmalloc pools. we
> >> don't free any objects from that path. just move them around within their
> >> size classes - to consolidate objects and to, may be, free unused pages
> >> [but we first need to make them "unused"]. it's not a mandatory thing for
> >> zsmalloc, we are just trying to be nice.
> > OK, it smells like an abuse of the API but please add a comment
> > clarifying that.
> >
> > Thanks!
> I can update the existing comment to be like that:
>          /*
>           * Not critical since shrinker is only used to trigger internal
>           * de-fragmentation of the pool which is pretty optional thing.
>           * If registration fails we still can use the pool normally and
>           * user can trigger compaction manually. Thus, ignore return code.
>           */
> 
> Sergey, does this sound well to you ? Or not clear enough, Michal ?

I did this:

From: Andrew Morton <akpm@linux-foundation.org>
Subject: mm-zsmalloc-simplify-shrinker-init-destroy-fix

update comment (Aliaksei), make zs_register_shrinker() return void

Cc: Aliaksei Karaliou <akaraliou.dev@gmail.com>
Cc: Michal Hocko <mhocko@kernel.org>
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 |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff -puN mm/zsmalloc.c~mm-zsmalloc-simplify-shrinker-init-destroy-fix mm/zsmalloc.c
--- a/mm/zsmalloc.c~mm-zsmalloc-simplify-shrinker-init-destroy-fix
+++ a/mm/zsmalloc.c
@@ -2323,14 +2323,14 @@ static void zs_unregister_shrinker(struc
 	unregister_shrinker(&pool->shrinker);
 }
 
-static int zs_register_shrinker(struct zs_pool *pool)
+static void zs_register_shrinker(struct zs_pool *pool)
 {
 	pool->shrinker.scan_objects = zs_shrinker_scan;
 	pool->shrinker.count_objects = zs_shrinker_count;
 	pool->shrinker.batch = 0;
 	pool->shrinker.seeks = DEFAULT_SEEKS;
 
-	return register_shrinker(&pool->shrinker);
+	register_shrinker(&pool->shrinker);
 }
 
 /**
@@ -2419,10 +2419,12 @@ struct zs_pool *zs_create_pool(const cha
 		goto err;
 
 	/*
-	 * Not critical, we still can use the pool
-	 * and user can trigger compaction manually.
+	 * Not critical since shrinker is only used to trigger internal
+	 * defragmentation of the pool which is pretty optional thing.  If
+	 * registration fails we still can use the pool normally and user can
+	 * trigger compaction manually. Thus, ignore return code.
 	 */
-	(void) zs_register_shrinker(pool);
+	zs_register_shrinker(pool);
 
 	return 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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Sergey Senozhatsky]

On (12/19/17 20:45), Aliaksei Karaliou wrote:
[..]
> > OK, it smells like an abuse of the API but please add a comment
> > clarifying that.
> > 
> > Thanks!
> I can update the existing comment to be like that:
>         /*
>          * Not critical since shrinker is only used to trigger internal
>          * de-fragmentation of the pool which is pretty optional thing.
>          * If registration fails we still can use the pool normally and
>          * user can trigger compaction manually. Thus, ignore return code.
>          */
> 
> Sergey, does this sound well to you ? Or not clear enough, Michal ?

looks good. thanks!

	-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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Sergey Senozhatsky]

On (12/19/17 15:27), Andrew Morton wrote:
> I did this:
> 
> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: mm-zsmalloc-simplify-shrinker-init-destroy-fix
> 
> update comment (Aliaksei), make zs_register_shrinker() return void
> 
> Cc: Aliaksei Karaliou <akaraliou.dev@gmail.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

looks good. thanks!

	-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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Sergey Senozhatsky]

Hi,

On (12/19/17 16:58), Michal Hocko wrote:
[..]
> > we use shrinker for "optional" de-fragmentation of zsmalloc pools. we
> > don't free any objects from that path. just move them around within their
> > size classes - to consolidate objects and to, may be, free unused pages
> > [but we first need to make them "unused"]. it's not a mandatory thing for
> > zsmalloc, we are just trying to be nice.
> 
> OK, it smells like an abuse of the API

we don't use shrinker callback to "just reduce the internal fragmentation".
the only reason we do de-fragmentation is to release the pages. if we see
that defragmentation is going to be a useless exercise and we are not going
to free pages, we just skip that class.

so at the end it's - the kernel asks us to shrink, we are trying to shrink
[release unneeded pages].

	-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 v2] mm/zsmalloc: simplify shrinker init/destroy

[A K]

On 12/20/2017 10:15 AM, Sergey Senozhatsky wrote:

>
> On (12/19/17 15:27), Andrew Morton wrote:
> > I did this:
> >
> > From: Andrew Morton <akpm@linux-foundation.org>
> > Subject: mm-zsmalloc-simplify-shrinker-init-destroy-fix
> >
> > update comment (Aliaksei), make zs_register_shrinker() return void
> >
> > Cc: Aliaksei Karaliou <akaraliou.dev@gmail.com>
> > Cc: Michal Hocko <mhocko@kernel.org>
> > Cc: Minchan Kim <minchan@kernel.org>
> > Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>
> looks good. thanks!
>
> 	-ss
Thanks for updating the code, Andrew, but removing return from
zs_register_shrinker() leads to triggering of 'warn_unused_result'
from 'sparse' on the line where register_shrinker() is called
(which was recently marker as __must_check).

May we leave previous variant to avoid that ? Or it is not critical ?

Best regards,
    Aliaksei.

--
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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Sergey Senozhatsky]

On (12/20/17 11:29), A K wrote:
[..]
> May we leave previous variant to avoid that ? Or it is not critical ?

let's keep void zs_register_shrinker() and just suppress the
register_shrinker() must_check warning.

	-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 v2] mm/zsmalloc: simplify shrinker init/destroy

[A K]

On 12/20/2017 11:34 AM, Sergey Senozhatsky wrote:

> On (12/20/17 11:29), A K wrote:
> [..]
>> May we leave previous variant to avoid that ? Or it is not critical ?
> let's keep void zs_register_shrinker() and just suppress the
> register_shrinker() must_check warning.
>
> 	-ss
IMHO, It seems that there is no obvious way to suppress this like casting to void.
We can probably add pr_debug/warn in order to use the result somehow.

Best regards,
    Aliaksei.

--
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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Michal Hocko]

On Wed 20-12-17 17:34:03, Sergey Senozhatsky wrote:
> On (12/20/17 11:29), A K wrote:
> [..]
> > May we leave previous variant to avoid that ? Or it is not critical ?
> 
> let's keep void zs_register_shrinker() and just suppress the
> register_shrinker() must_check warning.

I would just hope we simply drop the must_check nonsense.
-- 
Michal Hocko
SUSE Labs

--
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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Michal Hocko]

On Wed 20-12-17 18:16:53, Sergey Senozhatsky wrote:
> On (12/20/17 10:08), Michal Hocko wrote:
> [..]
> > > let's keep void zs_register_shrinker() and just suppress the
> > > register_shrinker() must_check warning.
> > 
> > I would just hope we simply drop the must_check nonsense.
> 
> agreed. given that unregister_shrinker() does not oops anymore,
> enforcing that check does not make that much sense.

Well, the registration failure is a failure like any others. Ignoring
the failure can have bad influence on the overal system behavior but
that is no different from thousands of other functions. must_check is an
overreaction here IMHO.
-- 
Michal Hocko
SUSE Labs

--
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 v2] mm/zsmalloc: simplify shrinker init/destroy

[A K]

On 12/20/2017 12:25 PM, Michal Hocko wrote:

> On Wed 20-12-17 18:16:53, Sergey Senozhatsky wrote:
>> On (12/20/17 10:08), Michal Hocko wrote:
>> [..]
>>>> let's keep void zs_register_shrinker() and just suppress the
>>>> register_shrinker() must_check warning.
>>> I would just hope we simply drop the must_check nonsense.
>> agreed. given that unregister_shrinker() does not oops anymore,
>> enforcing that check does not make that much sense.
> Well, the registration failure is a failure like any others. Ignoring
> the failure can have bad influence on the overal system behavior but
> that is no different from thousands of other functions. must_check is an
> overreaction here IMHO.
Fine, then I'll resend the patch with diff from Andrew, and also I'd like
to move the improved comment into zs_register_shrinker().

Best regards,
    Aliaksei.

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

[PATCH v3] mm/zsmalloc: simplify shrinker init/destroy

[Aliaksei Karaliou]

Structure zs_pool has special flag to indicate success of shrinker
initialization. unregister_shrinker() has improved and can detect
by itself whether actual deinitialization should be performed or not,
so extra flag becomes redundant.

Signed-off-by: Aliaksei Karaliou <akaraliou.dev@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
 mm/zsmalloc.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

v2: Added include <linux/shrinker.h> as suggested by Sergey Senozhatsky.
v3: Improved comment regarding shrinker registration failure.
    Added patch from Andrew Morton to make zs_register_shrinker() void.

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 685049a..bed387b 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -53,6 +53,7 @@
 #include <linux/mount.h>
 #include <linux/migrate.h>
 #include <linux/pagemap.h>
+#include <linux/shrinker.h>
 
 #define ZSPAGE_MAGIC	0x58
 
@@ -256,11 +257,7 @@ struct zs_pool {
 
 	/* Compact classes */
 	struct shrinker shrinker;
-	/*
-	 * To signify that register_shrinker() was successful
-	 * and unregister_shrinker() will not Oops.
-	 */
-	bool shrinker_enabled;
+
 #ifdef CONFIG_ZSMALLOC_STAT
 	struct dentry *stat_dentry;
 #endif
@@ -2323,20 +2320,23 @@ static unsigned long zs_shrinker_count(struct shrinker *shrinker,
 
 static void zs_unregister_shrinker(struct zs_pool *pool)
 {
-	if (pool->shrinker_enabled) {
-		unregister_shrinker(&pool->shrinker);
-		pool->shrinker_enabled = false;
-	}
+	unregister_shrinker(&pool->shrinker);
 }
 
-static int zs_register_shrinker(struct zs_pool *pool)
+static void zs_register_shrinker(struct zs_pool *pool)
 {
 	pool->shrinker.scan_objects = zs_shrinker_scan;
 	pool->shrinker.count_objects = zs_shrinker_count;
 	pool->shrinker.batch = 0;
 	pool->shrinker.seeks = DEFAULT_SEEKS;
 
-	return register_shrinker(&pool->shrinker);
+	/*
+	 * Not critical since shrinker is only used to trigger internal
+	 * defragmentation of the pool which is pretty optional thing.  If
+	 * registration fails we still can use the pool normally and user can
+	 * trigger compaction manually. Thus, ignore return code.
+	 */
+	register_shrinker(&pool->shrinker);
 }
 
 /**
@@ -2424,12 +2424,8 @@ struct zs_pool *zs_create_pool(const char *name)
 	if (zs_register_migration(pool))
 		goto err;
 
-	/*
-	 * Not critical, we still can use the pool
-	 * and user can trigger compaction manually.
-	 */
-	if (zs_register_shrinker(pool) == 0)
-		pool->shrinker_enabled = true;
+	zs_register_shrinker(pool);
+
 	return pool;
 
 err:
-- 
2.1.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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Tetsuo Handa]

On 2017/12/20 18:25, Michal Hocko wrote:
> On Wed 20-12-17 18:16:53, Sergey Senozhatsky wrote:
>> On (12/20/17 10:08), Michal Hocko wrote:
>> [..]
>>>> let's keep void zs_register_shrinker() and just suppress the
>>>> register_shrinker() must_check warning.
>>>
>>> I would just hope we simply drop the must_check nonsense.
>>
>> agreed. given that unregister_shrinker() does not oops anymore,
>> enforcing that check does not make that much sense.
> 
> Well, the registration failure is a failure like any others. Ignoring
> the failure can have bad influence on the overal system behavior but
> that is no different from thousands of other functions. must_check is an
> overreaction here IMHO.
> 

I don't think that must_check is an overreaction.
As of linux-next-20171218, no patch is available for 10 locations.

drivers/staging/android/ion/ion_heap.c:306:     register_shrinker(&heap->shrinker);
drivers/staging/android/ashmem.c:857:   register_shrinker(&ashmem_shrinker);
drivers/gpu/drm/ttm/ttm_page_alloc_dma.c:1185:  register_shrinker(&manager->mm_shrink);
drivers/gpu/drm/ttm/ttm_page_alloc.c:484:       register_shrinker(&manager->mm_shrink);
drivers/gpu/drm/i915/i915_gem_shrinker.c:508:   WARN_ON(register_shrinker(&i915->mm.shrinker));
drivers/gpu/drm/msm/msm_gem_shrinker.c:154:     WARN_ON(register_shrinker(&priv->shrinker));
drivers/md/dm-bufio.c:1756:     register_shrinker(&c->shrinker);
drivers/android/binder_alloc.c:1012:    register_shrinker(&binder_shrinker);
arch/x86/kvm/mmu.c:5485:        register_shrinker(&mmu_shrinker);
fs/xfs/xfs_qm.c:698:    register_shrinker(&qinf->qi_shrinker);

We have out of tree modules. And as a troubleshooting staff at
a support center, I want to be able to identify the careless module.

--
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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Michal Hocko]

On Wed 20-12-17 20:05:35, Tetsuo Handa wrote:
> On 2017/12/20 18:25, Michal Hocko wrote:
> > On Wed 20-12-17 18:16:53, Sergey Senozhatsky wrote:
> >> On (12/20/17 10:08), Michal Hocko wrote:
> >> [..]
> >>>> let's keep void zs_register_shrinker() and just suppress the
> >>>> register_shrinker() must_check warning.
> >>>
> >>> I would just hope we simply drop the must_check nonsense.
> >>
> >> agreed. given that unregister_shrinker() does not oops anymore,
> >> enforcing that check does not make that much sense.
> > 
> > Well, the registration failure is a failure like any others. Ignoring
> > the failure can have bad influence on the overal system behavior but
> > that is no different from thousands of other functions. must_check is an
> > overreaction here IMHO.
> > 
> 
> I don't think that must_check is an overreaction.
> As of linux-next-20171218, no patch is available for 10 locations.
> 
> drivers/staging/android/ion/ion_heap.c:306:     register_shrinker(&heap->shrinker);
> drivers/staging/android/ashmem.c:857:   register_shrinker(&ashmem_shrinker);
> drivers/gpu/drm/ttm/ttm_page_alloc_dma.c:1185:  register_shrinker(&manager->mm_shrink);
> drivers/gpu/drm/ttm/ttm_page_alloc.c:484:       register_shrinker(&manager->mm_shrink);
> drivers/gpu/drm/i915/i915_gem_shrinker.c:508:   WARN_ON(register_shrinker(&i915->mm.shrinker));
> drivers/gpu/drm/msm/msm_gem_shrinker.c:154:     WARN_ON(register_shrinker(&priv->shrinker));
> drivers/md/dm-bufio.c:1756:     register_shrinker(&c->shrinker);
> drivers/android/binder_alloc.c:1012:    register_shrinker(&binder_shrinker);
> arch/x86/kvm/mmu.c:5485:        register_shrinker(&mmu_shrinker);
> fs/xfs/xfs_qm.c:698:    register_shrinker(&qinf->qi_shrinker);

And how exactly has the must_check helped for those? Come on, start
being serious finally. This is a matter of fixing those. You have done
a good deal of work for some, it just takes to finish the rest. The
warning doesn't help on its own, it just makes people ignore it after
some time or make it silent in some way.

> We have out of tree modules. And as a troubleshooting staff at
> a support center, I want to be able to identify the careless module.

I do not care the slightest about those, to be honest. We assume a
certain level of sanity from _any_ code running in the kernel and
handling error paths properly is a part of that assumption.
-- 
Michal Hocko
SUSE Labs

--
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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Michal Hocko]

On Wed 20-12-17 12:38:35, Michal Hocko wrote:
> On Wed 20-12-17 20:05:35, Tetsuo Handa wrote:
> > On 2017/12/20 18:25, Michal Hocko wrote:
> > > On Wed 20-12-17 18:16:53, Sergey Senozhatsky wrote:
> > >> On (12/20/17 10:08), Michal Hocko wrote:
> > >> [..]
> > >>>> let's keep void zs_register_shrinker() and just suppress the
> > >>>> register_shrinker() must_check warning.
> > >>>
> > >>> I would just hope we simply drop the must_check nonsense.
> > >>
> > >> agreed. given that unregister_shrinker() does not oops anymore,
> > >> enforcing that check does not make that much sense.
> > > 
> > > Well, the registration failure is a failure like any others. Ignoring
> > > the failure can have bad influence on the overal system behavior but
> > > that is no different from thousands of other functions. must_check is an
> > > overreaction here IMHO.
> > > 
> > 
> > I don't think that must_check is an overreaction.
> > As of linux-next-20171218, no patch is available for 10 locations.
> > 
> > drivers/staging/android/ion/ion_heap.c:306:     register_shrinker(&heap->shrinker);
> > drivers/staging/android/ashmem.c:857:   register_shrinker(&ashmem_shrinker);
> > drivers/gpu/drm/ttm/ttm_page_alloc_dma.c:1185:  register_shrinker(&manager->mm_shrink);
> > drivers/gpu/drm/ttm/ttm_page_alloc.c:484:       register_shrinker(&manager->mm_shrink);
> > drivers/gpu/drm/i915/i915_gem_shrinker.c:508:   WARN_ON(register_shrinker(&i915->mm.shrinker));
> > drivers/gpu/drm/msm/msm_gem_shrinker.c:154:     WARN_ON(register_shrinker(&priv->shrinker));
> > drivers/md/dm-bufio.c:1756:     register_shrinker(&c->shrinker);
> > drivers/android/binder_alloc.c:1012:    register_shrinker(&binder_shrinker);
> > arch/x86/kvm/mmu.c:5485:        register_shrinker(&mmu_shrinker);
> > fs/xfs/xfs_qm.c:698:    register_shrinker(&qinf->qi_shrinker);
> 
> And how exactly has the must_check helped for those? Come on, start
> being serious finally. This is a matter of fixing those. You have done
> a good deal of work for some, it just takes to finish the rest. The
> warning doesn't help on its own, it just makes people ignore it after
> some time or make it silent in some way.

Also have a look at how WARN_ON simply papers over the wrong code and
must_check will not help you the slightest.
-- 
Michal Hocko
SUSE Labs

--
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 v2] mm/zsmalloc: simplify shrinker init/destroy

[Aliaksei Karaliou]

On 12/20/2017 02:57 PM, Michal Hocko wrote:
> On Wed 20-12-17 12:38:35, Michal Hocko wrote:
>> On Wed 20-12-17 20:05:35, Tetsuo Handa wrote:
>>> On 2017/12/20 18:25, Michal Hocko wrote:
>>>> On Wed 20-12-17 18:16:53, Sergey Senozhatsky wrote:
>>>>> On (12/20/17 10:08), Michal Hocko wrote:
>>>>> [..]
>>>>>>> let's keep void zs_register_shrinker() and just suppress the
>>>>>>> register_shrinker() must_check warning.
>>>>>> I would just hope we simply drop the must_check nonsense.
>>>>> agreed. given that unregister_shrinker() does not oops anymore,
>>>>> enforcing that check does not make that much sense.
>>>> Well, the registration failure is a failure like any others. Ignoring
>>>> the failure can have bad influence on the overal system behavior but
>>>> that is no different from thousands of other functions. must_check is an
>>>> overreaction here IMHO.
>>>>
>>> I don't think that must_check is an overreaction.
>>> As of linux-next-20171218, no patch is available for 10 locations.
>>>
>>> drivers/staging/android/ion/ion_heap.c:306:     register_shrinker(&heap->shrinker);
>>> drivers/staging/android/ashmem.c:857:   register_shrinker(&ashmem_shrinker);
>>> drivers/gpu/drm/ttm/ttm_page_alloc_dma.c:1185:  register_shrinker(&manager->mm_shrink);
>>> drivers/gpu/drm/ttm/ttm_page_alloc.c:484:       register_shrinker(&manager->mm_shrink);
>>> drivers/gpu/drm/i915/i915_gem_shrinker.c:508:   WARN_ON(register_shrinker(&i915->mm.shrinker));
>>> drivers/gpu/drm/msm/msm_gem_shrinker.c:154:     WARN_ON(register_shrinker(&priv->shrinker));
>>> drivers/md/dm-bufio.c:1756:     register_shrinker(&c->shrinker);
>>> drivers/android/binder_alloc.c:1012:    register_shrinker(&binder_shrinker);
>>> arch/x86/kvm/mmu.c:5485:        register_shrinker(&mmu_shrinker);
>>> fs/xfs/xfs_qm.c:698:    register_shrinker(&qinf->qi_shrinker);
>> And how exactly has the must_check helped for those? Come on, start
>> being serious finally. This is a matter of fixing those. You have done
>> a good deal of work for some, it just takes to finish the rest. The
>> warning doesn't help on its own, it just makes people ignore it after
>> some time or make it silent in some way.
> Also have a look at how WARN_ON simply papers over the wrong code and
> must_check will not help you the slightest.

Regarding the other locations where return code is ignored, I think I will
try to fix them as I did in Lustre code recently.
However, it might be not straightforward and zsmalloc is good example -
we understand that failure is not critical and we can live without shrinker.

Locations specified by Michal are also different, for example:

drivers/gpu/drm/i915/i915_gem_shrinker.c:508:   WARN_ON(register_shrinker(&i915->mm.shrinker));
- this change is intentional.
arch/x86/kvm/mmu.c:5485:        register_shrinker(&mmu_shrinker);
- was made before register_shrinker() became non-void.

and so on. The question is what to do in each particular case ?
Some people may consider wrapping it with WARN_ON to be rather good option too
while the others will prefer to consider it as a critical failure or at least
do their own logging, with still looks similar with WARN_ON for me imho.

For me, must_check looks like thing that works mostly for new code only,
but in this case it works like a trigger that forces people to act and fix
previously written code, but yes, all depends on attitude as Michal noticed.

Best regards,
	Aliaksei.


--
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 v3] mm/zsmalloc: simplify shrinker init/destroy

[Minchan Kim]

On Wed, Dec 20, 2017 at 01:21:49PM +0300, Aliaksei Karaliou wrote:
> Structure zs_pool has special flag to indicate success of shrinker
> initialization. unregister_shrinker() has improved and can detect
> by itself whether actual deinitialization should be performed or not,
> so extra flag becomes redundant.
> 
> Signed-off-by: Aliaksei Karaliou <akaraliou.dev@gmail.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Acked-by: Minchan Kim <minchan@kernel.org>

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>

Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy

[Michal Hocko]

On Thu 21-12-17 00:20:44, Aliaksei Karaliou wrote:
> 
> 
> On 12/20/2017 02:57 PM, Michal Hocko wrote:
> > On Wed 20-12-17 12:38:35, Michal Hocko wrote:
> > > On Wed 20-12-17 20:05:35, Tetsuo Handa wrote:
> > > > On 2017/12/20 18:25, Michal Hocko wrote:
> > > > > On Wed 20-12-17 18:16:53, Sergey Senozhatsky wrote:
> > > > > > On (12/20/17 10:08), Michal Hocko wrote:
> > > > > > [..]
> > > > > > > > let's keep void zs_register_shrinker() and just suppress the
> > > > > > > > register_shrinker() must_check warning.
> > > > > > > I would just hope we simply drop the must_check nonsense.
> > > > > > agreed. given that unregister_shrinker() does not oops anymore,
> > > > > > enforcing that check does not make that much sense.
> > > > > Well, the registration failure is a failure like any others. Ignoring
> > > > > the failure can have bad influence on the overal system behavior but
> > > > > that is no different from thousands of other functions. must_check is an
> > > > > overreaction here IMHO.
> > > > > 
> > > > I don't think that must_check is an overreaction.
> > > > As of linux-next-20171218, no patch is available for 10 locations.
> > > > 
> > > > drivers/staging/android/ion/ion_heap.c:306:     register_shrinker(&heap->shrinker);
> > > > drivers/staging/android/ashmem.c:857:   register_shrinker(&ashmem_shrinker);
> > > > drivers/gpu/drm/ttm/ttm_page_alloc_dma.c:1185:  register_shrinker(&manager->mm_shrink);
> > > > drivers/gpu/drm/ttm/ttm_page_alloc.c:484:       register_shrinker(&manager->mm_shrink);
> > > > drivers/gpu/drm/i915/i915_gem_shrinker.c:508:   WARN_ON(register_shrinker(&i915->mm.shrinker));
> > > > drivers/gpu/drm/msm/msm_gem_shrinker.c:154:     WARN_ON(register_shrinker(&priv->shrinker));
> > > > drivers/md/dm-bufio.c:1756:     register_shrinker(&c->shrinker);
> > > > drivers/android/binder_alloc.c:1012:    register_shrinker(&binder_shrinker);
> > > > arch/x86/kvm/mmu.c:5485:        register_shrinker(&mmu_shrinker);
> > > > fs/xfs/xfs_qm.c:698:    register_shrinker(&qinf->qi_shrinker);
> > > And how exactly has the must_check helped for those? Come on, start
> > > being serious finally. This is a matter of fixing those. You have done
> > > a good deal of work for some, it just takes to finish the rest. The
> > > warning doesn't help on its own, it just makes people ignore it after
> > > some time or make it silent in some way.
> > Also have a look at how WARN_ON simply papers over the wrong code and
> > must_check will not help you the slightest.
> 
> Regarding the other locations where return code is ignored, I think I will
> try to fix them as I did in Lustre code recently.

That would be really appreciated!

> However, it might be not straightforward and zsmalloc is good example -
> we understand that failure is not critical and we can live without shrinker.
> 
> Locations specified by Michal are also different, for example:
> 
> drivers/gpu/drm/i915/i915_gem_shrinker.c:508:   WARN_ON(register_shrinker(&i915->mm.shrinker));
> - this change is intentional.

I would be careful here. I believe that the WARN_ON is not a solution
here. i915 can allocated _a lot_ of memory IIRC so a missing shrinker can
be really problematic. Talk to i915 people.

> arch/x86/kvm/mmu.c:5485:        register_shrinker(&mmu_shrinker);
> - was made before register_shrinker() became non-void.

Yes this would be the case for the most shrinkers. Glauber simply didn't
bother to handle those because failing registration is basically
impossible.

> and so on. The question is what to do in each particular case ?

Bail out unless the shrinker is along the zsmalloc case.

> Some people may consider wrapping it with WARN_ON to be rather good option too
> while the others will prefer to consider it as a critical failure or at least
> do their own logging, with still looks similar with WARN_ON for me imho.

Talk to respective maintainers and they will give hints.
-- 
Michal Hocko
SUSE Labs

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