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