* [PATCH] mm/zsmalloc: simplify shrinker init/destroy @ 2017-12-19 9:21 Aliaksei Karaliou 2017-12-19 10:22 ` Sergey Senozhatsky 0 siblings, 1 reply; 27+ messages in thread From: Aliaksei Karaliou @ 2017-12-19 9:21 UTC (permalink / raw) To: minchan, ngupta, sergey.senozhatsky.work; +Cc: Aliaksei Karaliou, linux-mm 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> ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] mm/zsmalloc: simplify shrinker init/destroy 2017-12-19 9:21 [PATCH] mm/zsmalloc: simplify shrinker init/destroy Aliaksei Karaliou @ 2017-12-19 10:22 ` Sergey Senozhatsky 2017-12-19 10:49 ` [PATCH v2] " Aliaksei Karaliou 0 siblings, 1 reply; 27+ messages in thread From: Sergey Senozhatsky @ 2017-12-19 10:22 UTC (permalink / raw) To: Aliaksei Karaliou; +Cc: minchan, ngupta, sergey.senozhatsky.work, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-19 10:22 ` Sergey Senozhatsky @ 2017-12-19 10:49 ` Aliaksei Karaliou 2017-12-19 11:04 ` Sergey Senozhatsky 2017-12-19 15:13 ` Michal Hocko 0 siblings, 2 replies; 27+ messages in thread From: Aliaksei Karaliou @ 2017-12-19 10:49 UTC (permalink / raw) To: minchan, ngupta, sergey.senozhatsky.work; +Cc: Aliaksei Karaliou, linux-mm 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> ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-19 10:49 ` [PATCH v2] " Aliaksei Karaliou @ 2017-12-19 11:04 ` Sergey Senozhatsky 2017-12-19 11:56 ` Sergey Senozhatsky 2017-12-19 15:13 ` Michal Hocko 1 sibling, 1 reply; 27+ messages in thread From: Sergey Senozhatsky @ 2017-12-19 11:04 UTC (permalink / raw) To: Aliaksei Karaliou; +Cc: minchan, ngupta, sergey.senozhatsky.work, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-19 11:04 ` Sergey Senozhatsky @ 2017-12-19 11:56 ` Sergey Senozhatsky 0 siblings, 0 replies; 27+ messages in thread From: Sergey Senozhatsky @ 2017-12-19 11:56 UTC (permalink / raw) To: Minchan Kim, Andrew Morton Cc: Aliaksei Karaliou, ngupta, linux-mm, 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-19 10:49 ` [PATCH v2] " Aliaksei Karaliou 2017-12-19 11:04 ` Sergey Senozhatsky @ 2017-12-19 15:13 ` Michal Hocko 2017-12-19 15:25 ` Sergey Senozhatsky 1 sibling, 1 reply; 27+ messages in thread From: Michal Hocko @ 2017-12-19 15:13 UTC (permalink / raw) To: Aliaksei Karaliou Cc: minchan, ngupta, sergey.senozhatsky.work, linux-mm, Andrew Morton 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-19 15:13 ` Michal Hocko @ 2017-12-19 15:25 ` Sergey Senozhatsky 2017-12-19 15:41 ` Aliaksei Karaliou 2017-12-19 15:58 ` Michal Hocko 0 siblings, 2 replies; 27+ messages in thread From: Sergey Senozhatsky @ 2017-12-19 15:25 UTC (permalink / raw) To: Michal Hocko Cc: Aliaksei Karaliou, minchan, ngupta, sergey.senozhatsky.work, linux-mm, Andrew Morton 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-19 15:25 ` Sergey Senozhatsky @ 2017-12-19 15:41 ` Aliaksei Karaliou 2017-12-19 15:58 ` Michal Hocko 1 sibling, 0 replies; 27+ messages in thread From: Aliaksei Karaliou @ 2017-12-19 15:41 UTC (permalink / raw) To: Sergey Senozhatsky, Michal Hocko Cc: minchan, ngupta, sergey.senozhatsky.work, linux-mm, Andrew Morton 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-19 15:25 ` Sergey Senozhatsky 2017-12-19 15:41 ` Aliaksei Karaliou @ 2017-12-19 15:58 ` Michal Hocko 2017-12-19 17:45 ` Aliaksei Karaliou 2017-12-20 7:15 ` Sergey Senozhatsky 1 sibling, 2 replies; 27+ messages in thread From: Michal Hocko @ 2017-12-19 15:58 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Aliaksei Karaliou, minchan, ngupta, sergey.senozhatsky.work, linux-mm, Andrew Morton 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-19 15:58 ` Michal Hocko @ 2017-12-19 17:45 ` Aliaksei Karaliou 2017-12-19 23:27 ` Andrew Morton 2017-12-20 1:00 ` Sergey Senozhatsky 2017-12-20 7:15 ` Sergey Senozhatsky 1 sibling, 2 replies; 27+ messages in thread From: Aliaksei Karaliou @ 2017-12-19 17:45 UTC (permalink / raw) To: Michal Hocko, Sergey Senozhatsky Cc: minchan, ngupta, sergey.senozhatsky.work, linux-mm, Andrew Morton 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-19 17:45 ` Aliaksei Karaliou @ 2017-12-19 23:27 ` Andrew Morton 2017-12-20 1:00 ` Sergey Senozhatsky 2017-12-20 1:00 ` Sergey Senozhatsky 1 sibling, 1 reply; 27+ messages in thread From: Andrew Morton @ 2017-12-19 23:27 UTC (permalink / raw) To: Aliaksei Karaliou Cc: Michal Hocko, Sergey Senozhatsky, minchan, ngupta, sergey.senozhatsky.work, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-19 23:27 ` Andrew Morton @ 2017-12-20 1:00 ` Sergey Senozhatsky 0 siblings, 0 replies; 27+ messages in thread From: Sergey Senozhatsky @ 2017-12-20 1:00 UTC (permalink / raw) To: Andrew Morton Cc: Aliaksei Karaliou, Michal Hocko, Sergey Senozhatsky, minchan, ngupta, sergey.senozhatsky.work, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-19 17:45 ` Aliaksei Karaliou 2017-12-19 23:27 ` Andrew Morton @ 2017-12-20 1:00 ` Sergey Senozhatsky 1 sibling, 0 replies; 27+ messages in thread From: Sergey Senozhatsky @ 2017-12-20 1:00 UTC (permalink / raw) To: Aliaksei Karaliou Cc: Michal Hocko, Sergey Senozhatsky, minchan, ngupta, sergey.senozhatsky.work, linux-mm, Andrew Morton 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-19 15:58 ` Michal Hocko 2017-12-19 17:45 ` Aliaksei Karaliou @ 2017-12-20 7:15 ` Sergey Senozhatsky 2017-12-20 8:29 ` A K 1 sibling, 1 reply; 27+ messages in thread From: Sergey Senozhatsky @ 2017-12-20 7:15 UTC (permalink / raw) To: Michal Hocko Cc: Sergey Senozhatsky, Aliaksei Karaliou, minchan, ngupta, sergey.senozhatsky.work, linux-mm, Andrew Morton 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-20 7:15 ` Sergey Senozhatsky @ 2017-12-20 8:29 ` A K 2017-12-20 8:34 ` Sergey Senozhatsky 0 siblings, 1 reply; 27+ messages in thread From: A K @ 2017-12-20 8:29 UTC (permalink / raw) To: Sergey Senozhatsky, Andrew Morton Cc: Michal Hocko, Sergey Senozhatsky, minchan, ngupta, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-20 8:29 ` A K @ 2017-12-20 8:34 ` Sergey Senozhatsky 2017-12-20 8:53 ` A K 2017-12-20 9:08 ` Michal Hocko 0 siblings, 2 replies; 27+ messages in thread From: Sergey Senozhatsky @ 2017-12-20 8:34 UTC (permalink / raw) To: A K Cc: Sergey Senozhatsky, Andrew Morton, Michal Hocko, Sergey Senozhatsky, minchan, ngupta, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-20 8:34 ` Sergey Senozhatsky @ 2017-12-20 8:53 ` A K 2017-12-20 9:08 ` Michal Hocko 1 sibling, 0 replies; 27+ messages in thread From: A K @ 2017-12-20 8:53 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Andrew Morton, Michal Hocko, Sergey Senozhatsky, minchan, ngupta, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-20 8:34 ` Sergey Senozhatsky 2017-12-20 8:53 ` A K @ 2017-12-20 9:08 ` Michal Hocko [not found] ` <20171220091653.GE11774@jagdpanzerIV> 1 sibling, 1 reply; 27+ messages in thread From: Michal Hocko @ 2017-12-20 9:08 UTC (permalink / raw) To: Sergey Senozhatsky Cc: A K, Andrew Morton, Sergey Senozhatsky, minchan, ngupta, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <20171220091653.GE11774@jagdpanzerIV>]
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy [not found] ` <20171220091653.GE11774@jagdpanzerIV> @ 2017-12-20 9:25 ` Michal Hocko 2017-12-20 9:30 ` A K 2017-12-20 11:05 ` [PATCH v2] " Tetsuo Handa 0 siblings, 2 replies; 27+ messages in thread From: Michal Hocko @ 2017-12-20 9:25 UTC (permalink / raw) To: Sergey Senozhatsky Cc: A K, Andrew Morton, Sergey Senozhatsky, minchan, ngupta, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-20 9:25 ` Michal Hocko @ 2017-12-20 9:30 ` A K 2017-12-20 10:21 ` [PATCH v3] " Aliaksei Karaliou 2017-12-20 11:05 ` [PATCH v2] " Tetsuo Handa 1 sibling, 1 reply; 27+ messages in thread From: A K @ 2017-12-20 9:30 UTC (permalink / raw) To: Michal Hocko, Sergey Senozhatsky Cc: Andrew Morton, Sergey Senozhatsky, minchan, ngupta, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3] mm/zsmalloc: simplify shrinker init/destroy 2017-12-20 9:30 ` A K @ 2017-12-20 10:21 ` Aliaksei Karaliou 2017-12-21 2:29 ` Minchan Kim 0 siblings, 1 reply; 27+ messages in thread From: Aliaksei Karaliou @ 2017-12-20 10:21 UTC (permalink / raw) To: minchan, ngupta, sergey.senozhatsky.work, akpm Cc: Aliaksei Karaliou, linux-mm 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> ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v3] mm/zsmalloc: simplify shrinker init/destroy 2017-12-20 10:21 ` [PATCH v3] " Aliaksei Karaliou @ 2017-12-21 2:29 ` Minchan Kim 0 siblings, 0 replies; 27+ messages in thread From: Minchan Kim @ 2017-12-21 2:29 UTC (permalink / raw) To: Aliaksei Karaliou; +Cc: ngupta, sergey.senozhatsky.work, akpm, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-20 9:25 ` Michal Hocko 2017-12-20 9:30 ` A K @ 2017-12-20 11:05 ` Tetsuo Handa 2017-12-20 11:38 ` Michal Hocko 1 sibling, 1 reply; 27+ messages in thread From: Tetsuo Handa @ 2017-12-20 11:05 UTC (permalink / raw) To: Michal Hocko, Sergey Senozhatsky Cc: A K, Andrew Morton, Sergey Senozhatsky, minchan, ngupta, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-20 11:05 ` [PATCH v2] " Tetsuo Handa @ 2017-12-20 11:38 ` Michal Hocko 2017-12-20 11:57 ` Michal Hocko 0 siblings, 1 reply; 27+ messages in thread From: Michal Hocko @ 2017-12-20 11:38 UTC (permalink / raw) To: Tetsuo Handa Cc: Sergey Senozhatsky, A K, Andrew Morton, Sergey Senozhatsky, minchan, ngupta, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-20 11:38 ` Michal Hocko @ 2017-12-20 11:57 ` Michal Hocko 2017-12-20 21:20 ` Aliaksei Karaliou 0 siblings, 1 reply; 27+ messages in thread From: Michal Hocko @ 2017-12-20 11:57 UTC (permalink / raw) To: Tetsuo Handa Cc: Sergey Senozhatsky, A K, Andrew Morton, Sergey Senozhatsky, minchan, ngupta, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-20 11:57 ` Michal Hocko @ 2017-12-20 21:20 ` Aliaksei Karaliou 2017-12-21 7:25 ` Michal Hocko 0 siblings, 1 reply; 27+ messages in thread From: Aliaksei Karaliou @ 2017-12-20 21:20 UTC (permalink / raw) To: Michal Hocko, Tetsuo Handa Cc: Sergey Senozhatsky, Andrew Morton, Sergey Senozhatsky, minchan, ngupta, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] mm/zsmalloc: simplify shrinker init/destroy 2017-12-20 21:20 ` Aliaksei Karaliou @ 2017-12-21 7:25 ` Michal Hocko 0 siblings, 0 replies; 27+ messages in thread From: Michal Hocko @ 2017-12-21 7:25 UTC (permalink / raw) To: Aliaksei Karaliou Cc: Tetsuo Handa, Sergey Senozhatsky, Andrew Morton, Sergey Senozhatsky, minchan, ngupta, linux-mm 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2017-12-21 7:25 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-19 9:21 [PATCH] mm/zsmalloc: simplify shrinker init/destroy Aliaksei Karaliou 2017-12-19 10:22 ` Sergey Senozhatsky 2017-12-19 10:49 ` [PATCH v2] " Aliaksei Karaliou 2017-12-19 11:04 ` Sergey Senozhatsky 2017-12-19 11:56 ` Sergey Senozhatsky 2017-12-19 15:13 ` Michal Hocko 2017-12-19 15:25 ` Sergey Senozhatsky 2017-12-19 15:41 ` Aliaksei Karaliou 2017-12-19 15:58 ` Michal Hocko 2017-12-19 17:45 ` Aliaksei Karaliou 2017-12-19 23:27 ` Andrew Morton 2017-12-20 1:00 ` Sergey Senozhatsky 2017-12-20 1:00 ` Sergey Senozhatsky 2017-12-20 7:15 ` Sergey Senozhatsky 2017-12-20 8:29 ` A K 2017-12-20 8:34 ` Sergey Senozhatsky 2017-12-20 8:53 ` A K 2017-12-20 9:08 ` Michal Hocko [not found] ` <20171220091653.GE11774@jagdpanzerIV> 2017-12-20 9:25 ` Michal Hocko 2017-12-20 9:30 ` A K 2017-12-20 10:21 ` [PATCH v3] " Aliaksei Karaliou 2017-12-21 2:29 ` Minchan Kim 2017-12-20 11:05 ` [PATCH v2] " Tetsuo Handa 2017-12-20 11:38 ` Michal Hocko 2017-12-20 11:57 ` Michal Hocko 2017-12-20 21:20 ` Aliaksei Karaliou 2017-12-21 7:25 ` Michal Hocko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.