Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile

[Sergey Senozhatsky]

Hello Christoph,

On (07/11/15 03:02), Christoph Hellwig wrote:
> > 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 to make sure that "unregister_shrinker()"
> > will not blow up later
> > (b) be fishy (potentially can Oops)
> > (c) access private members `struct shrinker' (e.g. `shrink.list.next')
> 
> Ayone who does that is broken.  You just need to have clear init (with
> proper unwinding) and exit functions and order things properly.  It
> works like most register/unregister calls and should stay that way.
> 
> Maye you you should ty to explain what practical problem you're seeing
> to start with.

Yes, but the main difference here is that it seems that shrinker users
don't tend to treat shrinker registration failures as fatal errors and
just continue with shrinker functionality disabled. And it makes sense.

(copy paste from https://lkml.org/lkml/2015/7/9/751)

> Ayone who does that is broken

I'm afraid, in that case we almost don't have not-broken shrinker users.


-- ignoring register_shrinker() error

: 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);


-- and here

:void i915_gem_shrinker_init(struct drm_i915_private *dev_priv)
:{
:        dev_priv->mm.shrinker.scan_objects = i915_gem_shrinker_scan;
:        dev_priv->mm.shrinker.count_objects = i915_gem_shrinker_count;
:        dev_priv->mm.shrinker.seeks = DEFAULT_SEEKS;
:        register_shrinker(&dev_priv->mm.shrinker);
:
:        dev_priv->mm.oom_notifier.notifier_call = i915_gem_shrinker_oom;
:        register_oom_notifier(&dev_priv->mm.oom_notifier);
:}


-- and here

:int __init gfs2_glock_init(void)
:{
:        unsigned i;
...
:        register_shrinker(&glock_shrinker);
:
:        return 0;
:}
:
:void gfs2_glock_exit(void)
:{
:        unregister_shrinker(&glock_shrinker);
:        destroy_workqueue(glock_workqueue);
:        destroy_workqueue(gfs2_delete_workqueue);
:}


-- and here

:static int __init lowmem_init(void)
:{
:        register_shrinker(&lowmem_shrinker);
:        return 0;
:}
:
:static void __exit lowmem_exit(void)
:{
:        unregister_shrinker(&lowmem_shrinker);
:}



-- accessing private member 'c->shrink.list.next' to distinguish between
'register_shrinker() was successful and need to unregister it' and
'register_shrinker() failed, don't unregister_shrinker() because it
may Oops'

: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);


-- and here
:int bch_btree_cache_alloc(struct cache_set *c)
:{
...
:        register_shrinker(&c->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);
:


And so on and on.

In fact, 'git grep = register_shrinker' gives only

$ git grep '= register_shrinker'
fs/ext4/extents_status.c:       err = register_shrinker(&sbi->s_es_shrinker);
fs/nfsd/nfscache.c:     status = register_shrinker(&nfsd_reply_cache_shrinker);
fs/ubifs/super.c:       err = register_shrinker(&ubifs_shrinker_info);
mm/huge_memory.c:       err = register_shrinker(&huge_zero_page_shrinker);
mm/workingset.c:        ret = register_shrinker(&workingset_shadow_shrinker);


The rest is 'broken'.

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