Re: [PATCH] mm/vmscan: make do_shrink_slab more robust.

[<jiang.biao2@zte.com.cn>]

[-- Attachment #1.1: Type: text/plain, Size: 2048 bytes --]

> On Mon, Nov 27, 2017 at 12:46:27PM +0800, jiang.biao2@zte.com.cn wrote:> > On Mon, Nov 27, 2017 at 09:37:30AM +0800, Jiang Biao wrote:> >
> > > > This patch make do_shrink_slab more robust when
> > > > shrinker->count_objects return negative freeable.
> > >
> > > Shrinker.h says count_objects should return 0 if there are no
> > > freeable objects, not -1.
> > >
> > > So if something returns -1, changing it with returning 0 would
> > be more proper fix.
> > >
> > Hi,
> > Indeed it's not a bug of vmscan, but there are many shrinkers
> > out there, which may return negative value unwillingly(in some
> > rare cases, such as decreasing cocurrently). It's unlikely and
> > should be avioded, but not impossible, this patch may make it
> > more robust and could not hurt :).
> 
> Yub, I'm not strong against of your claim. However, let's think
> from different point of view.
> 
> API says it should return 0 unless shrinker cannot find freeable
> object any more but with your change, implmentation handles
> although a shrinker return minus value by mistake.
> 
> In future, MM guys might want to extend count_objects returning
> -ERR_SOMETHING to propagate error, for example but we cannot.
> Because some of shrinkers already rely on the implementation so
> if we start to support minus value return, some of shrinker might
> be broken.
> 
> Yes, it's the imaginary scenario but wanted why such changes
> makes us trouble in future, API PoV.
I agree with your concern.  How about we take another way by 
adding some warning in such case? such as,
        freeable = shrinker->count_objects(shrinker, shrinkctl);
+       if (unlikely(freeable < 0)) {
+               pr_err("shrink_slab: %pF negative objects returned. freeable=%ld\n",
+                       shrinker->scan_objects, freeable);
+               freeable = 0;  //maybe not needed?
+       }
        if (freeable == 0)
                return 0;
In this way, we would not break the API, but could alert user exception 
with message, and make it more robust in such case.

Thanks.