mm/zsmalloc: simplify shrinker init/destroy

PATCH Superseded Aliaksei Karaliou 7 participants 27 messages Dec 19, 2017 – Dec 21, 2017 1 message not in the archive

Subsystems (2)

Memory management Zsmalloc compressed slab memory allocator

lore

Share & tools

lore

Aliaksei Karaliou

author

2017-12-19 09:21 UTC #0 lore raw

PATCH 1/1 mm/zsmalloc: simplify shrinker init/destroy

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 <[email protected]>

---
 mm/zsmalloc.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

mm/zsmalloc.c +4 ~~-11~~

index 685049a..6b6aaa9 100644
@@ -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:

Signature · 7 lines

2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Sergey Senozhatsky

2017-12-19 10:22 UTC #1 lore raw

Hi,

Quoted 3 lines · Aliaksei Karaliou

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

Quoted 1 line

Signed-off-by Aliaksei Karaliou <[email protected]>

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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Aliaksei Karaliou

author

2017-12-19 10:49 UTC #2 lore raw

PATCH v2 1/1 mm/zsmalloc: simplify shrinker init/destroy

Signed-off-by Aliaksei Karaliou <[email protected]>

---
 mm/zsmalloc.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

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

mm/zsmalloc.c +5 ~~-11~~

index 685049a..628a1bc 100644
@@ -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:

Signature · 7 lines

2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Sergey Senozhatsky

Reviewed-by on v2 1/1

2017-12-19 11:04 UTC #3 lore raw

Ccing Andrew [email protected]

Quoted 7 lines · 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 <[email protected]>

looks good to me.

Reviewed-by Sergey Senozhatsky <[email protected]>

-ss

Sergey Senozhatsky

Reviewed-by

2017-12-19 11:56 UTC #4 lore raw

d'oh... actually Ccing Andrew.

-ss

---

Ccing Andrew [email protected]

Quoted 7 lines · 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 <[email protected]>

looks good to me.

Reviewed-by Sergey Senozhatsky <[email protected]>

Michal Hocko

2017-12-19 15:13 UTC #5 lore raw

In reply to Aliaksei Karaliou

On Tue 19-12-17 13:49:12, Aliaksei Karaliou wrote:
[...]

mm/zsmalloc.c struct zs_pool *zs_create_pool(const char *name) · line 2427

Quoted 9 lines

mm/zsmalloc.c +2 -2

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

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Sergey Senozhatsky

2017-12-19 15:25 UTC #6 lore raw

Hi Michal,

Quoted 14 lines · Michal Hocko

On Tue 19-12-17 13:49:12, Aliaksei Karaliou wrote:
[...]
mm/zsmalloc.c struct zs_pool *zs_create_pool(const char *name) · line 2427
Quoted 9 lines
mm/zsmalloc.c +2 -2
@@ -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

Aliaksei Karaliou

author

2017-12-19 15:41 UTC #7 lore raw

Quoted 16 lines · Sergey Senozhatsky

Hi Michal,
Quoted 13 lines · Michal Hocko
On Tue 19-12-17 13:49:12, Aliaksei Karaliou wrote:
[...]
mm/zsmalloc.c struct zs_pool *zs_create_pool(const char *name) · line 2427
Quoted 9 lines
mm/zsmalloc.c +2 -2
@@ -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?

Quoted 7 lines

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.

Michal Hocko

2017-12-19 15:58 UTC #8 lore raw

In reply to Sergey Senozhatsky

Quoted 18 lines · Sergey Senozhatsky

Hi Michal,
Quoted 14 lines · Michal Hocko
On Tue 19-12-17 13:49:12, Aliaksei Karaliou wrote:
[...]
mm/zsmalloc.c struct zs_pool *zs_create_pool(const char *name) · line 2427
Quoted 9 lines
mm/zsmalloc.c +2 -2
@@ -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?

Quoted 5 lines

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!

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Aliaksei Karaliou

author

2017-12-19 17:45 UTC #9 lore raw

Quoted 22 lines · Michal Hocko

Quoted 16 lines · Sergey Senozhatsky
Hi Michal,
Quoted 13 lines · Michal Hocko
On Tue 19-12-17 13:49:12, Aliaksei Karaliou wrote:
[...]
mm/zsmalloc.c struct zs_pool *zs_create_pool(const char *name) · line 2427
Quoted 9 lines
mm/zsmalloc.c +2 -2
@@ -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?
Quoted 5 lines
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.

Quoted 4 lines

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

Andrew Morton

maintainer

2017-12-19 23:27 UTC #10 lore raw

Quoted 19 lines · On Tue, 19 Dec 2017 20:45:17 +0300 Aliaksei Karaliou <[email protected]>…

Quoted 11 lines
Quoted 7 lines
Quoted 2 lines
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. */

Quoted 1 line

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

I did this:

From: Andrew Morton <[email protected]>
Subject: mm-zsmalloc-simplify-shrinker-init-destroy-fix

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

Cc Aliaksei Karaliou <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Minchan Kim <[email protected]>

Cc Sergey Senozhatsky <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 mm/zsmalloc.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

mm/zsmalloc.c renamed from mm/zsmalloc.c~mm-zsmalloc-simplify-shrinker-init-destroy-fix +7 -5

diff -puN mm/zsmalloc.c~mm-zsmalloc-simplify-shrinker-init-destroy-fix 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;

Sergey Senozhatsky

2017-12-20 01:01 UTC #11 lore raw

Quoted 5 lines · Andrew Morton

I did this:
From: Andrew Morton <[email protected]>
Subject: mm-zsmalloc-simplify-shrinker-init-destroy-fix

Quoted 7 lines

update comment (Aliaksei), make zs_register_shrinker() return void
Cc Aliaksei Karaliou <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Minchan Kim <[email protected]>
Cc Sergey Senozhatsky <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

looks good. thanks!

-ss

Sergey Senozhatsky

2017-12-20 01:00 UTC #12 lore raw

In reply to Aliaksei Karaliou

On (12/19/17 20:45), Aliaksei Karaliou wrote:
[..]

Quoted 12 lines

Quoted 4 lines
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. */

Quoted 1 line

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

looks good. thanks!

-ss

Sergey Senozhatsky

2017-12-20 07:15 UTC #13 lore raw

In reply to Michal Hocko

Hi,

On (12/19/17 16:58), Michal Hocko wrote:
[..]

Quoted 7 lines

Quoted 5 lines
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

A K

author

2017-12-20 08:29 UTC #14 lore raw

Quoted 15 lines · Sergey Senozhatsky

Quoted 5 lines · Andrew Morton
I did this:
From: Andrew Morton <[email protected]>
Subject: mm-zsmalloc-simplify-shrinker-init-destroy-fix
Quoted 7 lines
update comment (Aliaksei), make zs_register_shrinker() return void
Cc Aliaksei Karaliou <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Minchan Kim <[email protected]>
Cc Sergey Senozhatsky <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

Quoted 3 lines

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.

Sergey Senozhatsky

2017-12-20 08:34 UTC #15 lore raw

On (12/20/17 11:29), A K wrote:
[..]

Quoted 1 line

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

A K

author

2017-12-20 08:54 UTC #16 lore raw

Quoted 7 lines · Sergey Senozhatsky

On (12/20/17 11:29), A K wrote:
[..]
Quoted 1 line
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.

Michal Hocko

2017-12-20 09:08 UTC #17 lore raw

In reply to Sergey Senozhatsky

Quoted 6 lines · Sergey Senozhatsky

On (12/20/17 11:29), A K wrote:
[..]
Quoted 1 line
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.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

A message that is not in the archive (sent off-list or not yet received): 20171220091653.GE11774@jagdpanzerIV

Michal Hocko

2017-12-20 09:25 UTC #19 lore raw

Quoted 9 lines · Sergey Senozhatsky

On (12/20/17 10:08), Michal Hocko wrote:
[..]
Quoted 4 lines
Quoted 2 lines
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.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

A K

author

2017-12-20 09:30 UTC #20 lore raw

Quoted 5 lines · Michal Hocko

Quoted 4 lines · Sergey Senozhatsky
On (12/20/17 10:08), Michal Hocko wrote:
[..]
Quoted 2 lines
Quoted 2 lines
let's keep void zs_register_shrinker() and just suppress the register_shrinker() must_check warning.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
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.

Aliaksei Karaliou

author

2017-12-20 10:22 UTC #21 lore raw

PATCH v3 1/1 mm/zsmalloc: simplify shrinker init/destroy

Signed-off-by Aliaksei Karaliou <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

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

mm/zsmalloc.c +13 ~~-17~~

index 685049a..bed387b 100644
@@ -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:

Signature · 7 lines

2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Minchan Kim

maintainer Acked-by on v3 1/1

2017-12-21 02:29 UTC #22 lore raw

Quoted 7 lines · 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 <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

Acked-by Minchan Kim <[email protected]>

Thanks.

Tetsuo Handa

2017-12-20 11:05 UTC #23 lore raw

In reply to Michal Hocko

Quoted 8 lines · Michal Hocko

Quoted 7 lines · Sergey Senozhatsky
On (12/20/17 10:08), Michal Hocko wrote:
[..]
Quoted 4 lines
Quoted 2 lines
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.

Quoted 8 lines

Quoted 2 lines
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.

Michal Hocko

2017-12-20 11:38 UTC #24 lore raw

Quoted 23 lines · Tetsuo Handa

Quoted 8 lines · Michal Hocko
Quoted 7 lines · Sergey Senozhatsky
On (12/20/17 10:08), Michal Hocko wrote:
[..]
Quoted 4 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 2 lines
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);

Quoted 8 lines

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.

Quoted 2 lines

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.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michal Hocko

2017-12-20 11:57 UTC #25 lore raw

Quoted 33 lines · Michal Hocko

Quoted 23 lines · Tetsuo Handa
Quoted 8 lines · Michal Hocko
Quoted 7 lines · Sergey Senozhatsky
On (12/20/17 10:08), Michal Hocko wrote:
[..]
Quoted 4 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 2 lines
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);
Quoted 8 lines
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);

Quoted 5 lines

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.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Aliaksei Karaliou

author

2017-12-20 21:20 UTC #26 lore raw

Quoted 29 lines · Michal Hocko

Quoted 28 lines · Michal Hocko
Quoted 27 lines · Tetsuo Handa
Quoted 5 lines · Michal Hocko
Quoted 4 lines · Sergey Senozhatsky
On (12/20/17 10:08), Michal Hocko wrote:
[..]
Quoted 2 lines
Quoted 2 lines
let's keep void zs_register_shrinker() and just suppress the register_shrinker() must_check warning.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
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);

Quoted 7 lines

Quoted 5 lines
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.

Michal Hocko

2017-12-21 07:25 UTC #27 lore raw

Quoted 37 lines · Aliaksei Karaliou

Quoted 34 lines · Michal Hocko
Quoted 33 lines · Michal Hocko
Quoted 19 lines · Tetsuo Handa
Quoted 5 lines · Michal Hocko
Quoted 4 lines · Sergey Senozhatsky
On (12/20/17 10:08), Michal Hocko wrote:
[..]
Quoted 2 lines
Quoted 2 lines
let's keep void zs_register_shrinker() and just suppress the register_shrinker() must_check warning.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
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);
Quoted 8 lines
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.

Quoted 5 lines

Quoted 2 lines
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!

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 1 line

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

Bail out unless the shrinker is along the zsmalloc case.

Quoted 3 lines

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.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>