Re: [PATCH v2 1/2] mm,vmscan: Make unregister_shrinker() no-op if register_shrinker() failed.

[Michal Hocko <mhocko@kernel.org>]

On Sat 25-11-17 10:40:13, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Fri 24-11-17 22:21:55, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > > Since we can encourage register_shrinker() callers to check for failure
> > > > > by marking register_shrinker() as __must_check, unregister_shrinker()
> > > > > can stay silent.
> > > > 
> > > > I am not sure __must_check is the right way. We already do get
> > > > allocation warning if the registration fails so silent unregister is
> > > > acceptable. Unchecked register_shrinker is a bug like any other
> > > > unchecked error path.
> > > 
> > > I consider that __must_check is the simplest way to find all of
> > > unchecked register_shrinker bugs. Why not to encourage users to fix?
> > 
> > because git grep doesn't require to patch the kernel and still provide
> > the information you want.
> 
> I can't interpret this line. How git grep relevant?

you do not have to compile to see who is checking the return value.
Seriously there is no need to overcomplicate this. Newly added shrinkers
know the function returns might fail so we just have to handle existing
users and there are not all that many of those.

> If all register_shrinker() users were careful enough to check for git history
> everytime, we would not have come to current code. It is duty of patch author
> to take necessary precautions (for in-tree code) when some API starts to
> return an error which previously did not return an error. In this case, it is
> duty of author of commit 1d3d4437eae1bb29 ("vmscan: per-node deferred work").

Yes I agree, the change was pushed without a due review and necessary
changes.

-- 
Michal Hocko
SUSE Labs

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