Re: [PATCH] mm,vmscan: Mark register_shrinker() as __must_check

From: Dave Chinner <david@fromorbit.com>
To: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: akpm@linux-foundation.org, glauber@scylladb.com,
	mhocko@kernel.org, linux-mm@kvack.org, viro@zeniv.linux.org.uk,
	jack@suse.com, pbonzini@redhat.com, airlied@linux.ie,
	alexander.deucher@amd.com, shli@fb.com, snitzer@redhat.com
Subject: Re: [PATCH] mm,vmscan: Mark register_shrinker() as __must_check
Date: Thu, 23 Nov 2017 07:39:07 +1100	[thread overview]
Message-ID: <20171122203907.GI4094@dastard> (raw)
In-Reply-To: <201711221953.IDJ12440.OQLtFVOJFMSHFO@I-love.SAKURA.ne.jp>

On Wed, Nov 22, 2017 at 07:53:59PM +0900, Tetsuo Handa wrote:
> Tetsuo Handa wrote:
> > Andrew Morton wrote:
> > > On Tue, 21 Nov 2017 21:02:37 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> > > 
> > > > There are users not checking for register_shrinker() failure.
> > > > Continuing with ignoring failure can lead to later oops at
> > > > unregister_shrinker().
> > > > 
> > > > ...
> > > >
> > > > --- a/include/linux/shrinker.h
> > > > +++ b/include/linux/shrinker.h
> > > > @@ -75,6 +75,6 @@ struct shrinker {
> > > >  #define SHRINKER_NUMA_AWARE	(1 << 0)
> > > >  #define SHRINKER_MEMCG_AWARE	(1 << 1)
> > > >  
> > > > -extern int register_shrinker(struct shrinker *);
> > > > +extern __must_check int register_shrinker(struct shrinker *);
> > > >  extern void unregister_shrinker(struct shrinker *);
> > > >  #endif
> > > 
> > > hm, well, OK, it's a small kmalloc(GFP_KERNEL).  That won't be
> > > failing.
> > 
> > It failed by fault injection and resulted in a report at
> > http://lkml.kernel.org/r/001a113f996099503a055e793dd3@google.com .
> 
> Since kzalloc() can become > 32KB allocation if CONFIG_NODES_SHIFT > 12
> (which might not be impossible in near future), register_shrinker() can
> potentially become a costly allocation which might fail without invoking
> the OOM killer. It is a good opportunity to think whether we should allow
> register_shrinker() to fail.

Just fix the numa aware shrinkers, as they are the only ones that
will have this problem. There are only 6 of them, and only the 3
that existed at the time that register_shrinker() was changed to
return an error fail to check for an error. i.e. the superblock
shrinker, the XFS dquot shrinker and the XFS buffer cache shrinker.

Seems pretty straight forward to me....

> > > Affected code seems to be fs/xfs, fs/super.c, fs/quota,
> > > arch/x86/kvm/mmu, drivers/gpu/drm/ttm, drivers/md and a bunch of
> > > staging stuff.
> > > 
> > > I'm not sure this is worth bothering about?
> > > 
> > 
> > Continuing with failed register_shrinker() is almost always wrong.
> > Though I don't know whether mm/zsmalloc.c case can make sense.
> > 
> 
> Thinking from the fact that register_shrinker() had been "void" until Linux 3.11
> and we did not take appropriate precautions when changing to "int" in Linux 3.12,
> we need to consider making register_shrinker() "void" again.
> 
> If we could agree with opening up the use of __GFP_NOFAIL for allocating a few
> non-contiguous pages on large systems, we can make register_shrinker() "void"
> again. (Draft patch is shown below. I choose array of kmalloc(PAGE_SIZE)
> rather than kvmalloc() in order to use __GFP_NOFAIL.)

That's insane. NACK.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>