Re: [PATCH v6 7/7] zsmalloc: use shrinker to trigger auto-compaction

[Sergey Senozhatsky]

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



So... options:

(a) we need something to signify that zs_unregister_shrinker() was successful

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/