> On Mon, Nov 27, 2017 at 12:46:27PM +0800, jiang.biao2@xxxxxxxxxx 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.
