Hi Sergey,
On Wed, Jul 08, 2015 at 11:18:36AM +0900, Sergey Senozhatsky wrote:
> On (07/08/15 00:12), Sergey Senozhatsky wrote:
> > > I don't think it would fail in *real practice*.
> > > Althout it might happen, what does zram could help in that cases?
> > >
> >
> > This argument depends on the current register_shrinker() implementation,
> > should some one add additional return branch there and it's done.
> >
> > > If it were failed, it means there is already little memory on the system
> > > so zram could not be helpful for those environment.
> > > IOW, zram should be enabled earlier.
> > >
> > > If you want it strongly, please reproduce such failing and prove that
> > > zram was helpful for the system.
> >
> > No, thanks. I'll just remove it.
> >
>
> hm... This makes error path a bit ugly. What we have now is
> pretty straight forward
>
> ... zs_create_pool(char *name, gfp_t flags)
> {
> ..
> if (zs_register_shrinker(pool) == 0)
> pool->shrinker_enabled = true;
> ..
> err:
> zs_destroy_pool(pool);
> return NULL;
> }
>
> zs_destroy_pool() does a destruction. It performs unconditional
> zs_unregister_shrinker(), which does unregister_shrinker() _if needed_.
>
> Shrinker API does not handle nicely unregister_shrinker() on a not-registered
> ->shrinker. And error path can be triggered even before we do
> register_shrinker(),
> so we can't 'fix' unregister_shrinker() in a common way, doing something like
>
> void unregister_shrinker(struct shrinker *shrinker)
> {
> + if (!unlikely(shrinker->nr_deferred))
> + return;
> +
> down_write(&shrinker_rwsem);
> list_del(&shrinker->list);
> up_write(&shrinker_rwsem);
>
>
> (just for example), because someone can accidentally pass a dirty (not zeroed
> out) `struct shrinker'. e.g.
>
> struct foo {
> const char *b;
> ...
> struct shrinker s;
> };
>
> void bar(void)
> {
> struct foo *f = kmalloc(...);
>
> if (!f)
> return;
>
> f->a = kmalloc(...);
> if (!f->a)
> goto err;
>
> err:
> unregister_shrinker(f->s);
> ^^^^^^ boom
> ...
> }
>
>
Yes, it's ugly.
>
> So... options:
>
> (a) we need something to signify that zs_unregister_shrinker() was successful
I think a) is simple way to handle it now.
I don't want to stuck with this issue.
Please comment out why we need such boolean so after someone who has interest
on shrinker clean-up is able to grab a chance.
Thanks!
>
> or
>
> (b) factor out 'core' part of zs_destroy_pool() and do a full destruction when
> called from the outside (from zram for example), or a partial destruction when
> called from zs_create_pool() error path.
>
>
>
> or
>
> (c) introduce 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)
>
>
> and do
>
> struct zs_pool *zs_create_pool(char *name, gfp_t flags)
> {
> ..
> INIT_SHRINKER(&pool->shrinker);
>
> pool->name = kstrdup(name, GFP_KERNEL);
> ..
> }
>
>
>
> Looking at shrinker users, they all have to carry on some sort of
> a flag telling that "unregister_shrinker()" will not blow up... or
> just be fishy... like
>
> 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 access private members of the `struct shrinker', like
>
>
> 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.
>
> -ss
--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
