Hello, Forked from http://www.gossamer-threads.com/lists/linux/kernel/2209873#2209873 with some adjustments. Shrinker API does not handle nicely unregister_shrinker() on a not-registered ->shrinker. Looking at shrinker users, they all have to (a) carry on some sort of a flag telling that "unregister_shrinker()" will not blow up... or (b) just be fishy : int ldlm_pools_init(void) : { : int rc; : : rc = ldlm_pools_thread_start(); : if (rc == 0) { : register_shrinker(&ldlm_pools_srv_shrinker); : register_shrinker(&ldlm_pools_cli_shrinker); : } : return rc; : } : EXPORT_SYMBOL(ldlm_pools_init); : : void ldlm_pools_fini(void) : { : unregister_shrinker(&ldlm_pools_srv_shrinker); : unregister_shrinker(&ldlm_pools_cli_shrinker); : ldlm_pools_thread_stop(); : } : EXPORT_SYMBOL(ldlm_pools_fini); or (c) access private members `struct shrinker' :struct cache_set { : ... : struct shrinker shrink; : ... :}; : : ... : : void bch_btree_cache_free(struct cache_set *c) : { : struct btree *b; : struct closure cl; : closure_init_stack(&cl); : : if (c->shrink.list.next) : unregister_shrinker(&c->shrink); Note that `shrink.list.next' check. We can't `fix' unregister_shrinker() (by looking at some flag or checking `!shrinker->nr_deferred'), simply because someone can do something like this: :struct foo { : const char *b; : ... : struct shrinker s; :}; : :void bar(void) :{ : struct foo *f = kmalloc(...); /* or kzalloc() to NULL deref it*/ : : if (!f) : return; : : f->a = kmalloc(...); : if (!f->a) : goto err; : ... : register_shrinker(...); : ... : return; : :err: : unregister_shrinker(&f->s); : ^^^^^^ boom : ... :} Passing a `garbaged' or zeroed out `struct shrinker' to unregister_shrinker() :void unregister_shrinker(struct shrinker *shrinker) :{ : down_write(&shrinker_rwsem); : list_del(&shrinker->list); : up_write(&shrinker_rwsem); : kfree(shrinker->nr_deferred); :} I was thinking of a trivial INIT_SHRINKER macro to init `struct shrinker' internal members (composed in email client, not tested) include/linux/shrinker.h #define INIT_SHRINKER(s) \ do { \ (s)->nr_deferred = NULL; \ INIT_LIST_HEAD(&(s)->list); \ } while (0) Of course, every shrinker user need to INIT_SHRINKER() early enough to guarantee that unregister_shrinker() will be legal should anything go wrong. Example: :struct zs_pool *zs_create_pool(char *name, gfp_t flags) :{ : .. :+ INIT_SHRINKER(&pool->shrinker); : : pool->name = kstrdup(name, GFP_KERNEL); : if (!pool->name) goto err; : .. : register_shrinker(&pool->shrinker); : .. : return pool; : :err: : unregister_shrinker(&pool->shrinker); : .. :} Not much better, but at least some hacks can be avoided and accidental unregister_shrinker() happening in error path is safe now. How does it sound? --- include/linux/shrinker.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h index 4fcacd9..10adfc2 100644 --- a/include/linux/shrinker.h +++ b/include/linux/shrinker.h @@ -63,6 +63,12 @@ struct shrinker { }; #define DEFAULT_SEEKS 2 /* A good number if you don't know better. */ +#define INIT_SHRINKER(s) \ + do { \ + INIT_LIST_HEAD(&(s)->list); \ + (s)->nr_deferred = NULL; \ + } while (0) + /* Flags */ #define SHRINKER_NUMA_AWARE (1 << 0) #define SHRINKER_MEMCG_AWARE (1 << 1) -- 2.4.5 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>