[Intel-gfx] [PATCH 1/2] mm: Track actual nr_scanned during shrink_slab()

[Minchan Kim]

Hi Vlastimil,

On Thu, Aug 24, 2017 at 10:00:49AM +0200, Vlastimil Babka wrote:
> On 08/24/2017 07:11 AM, Minchan Kim wrote:
> > Hello Chris,
> > 
> > On Tue, Aug 22, 2017 at 02:53:24PM +0100, Chris Wilson wrote:
> >> Some shrinkers may only be able to free a bunch of objects at a time, and
> >> so free more than the requested nr_to_scan in one pass.
> 
> Can such shrinkers reflect that in their shrinker->batch value? Or is it
> unpredictable for each scan?
> 
> >> Whilst other
> >> shrinkers may find themselves even unable to scan as many objects as
> >> they counted, and so underreport. Account for the extra freed/scanned
> >> objects against the total number of objects we intend to scan, otherwise
> >> we may end up penalising the slab far more than intended. Similarly,
> >> we want to add the underperforming scan to the deferred pass so that we
> >> try harder and harder in future passes.
> >>
> >> v2: Andrew's shrinkctl->nr_scanned
> >>
> >> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >> Cc: Andrew Morton <akpm at linux-foundation.org>
> >> Cc: Michal Hocko <mhocko at suse.com>
> >> Cc: Johannes Weiner <hannes at cmpxchg.org>
> >> Cc: Hillf Danton <hillf.zj at alibaba-inc.com>
> >> Cc: Minchan Kim <minchan at kernel.org>
> >> Cc: Vlastimil Babka <vbabka at suse.cz>
> >> Cc: Mel Gorman <mgorman at techsingularity.net>
> >> Cc: Shaohua Li <shli at fb.com>
> >> Cc: linux-mm at kvack.org
> >> ---
> >>  include/linux/shrinker.h | 7 +++++++
> >>  mm/vmscan.c              | 7 ++++---
> >>  2 files changed, 11 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> >> index 4fcacd915d45..51d189615bda 100644
> >> --- a/include/linux/shrinker.h
> >> +++ b/include/linux/shrinker.h
> >> @@ -18,6 +18,13 @@ struct shrink_control {
> >>  	 */
> >>  	unsigned long nr_to_scan;
> >>  
> >> +	/*
> >> +	 * How many objects did scan_objects process?
> >> +	 * This defaults to nr_to_scan before every call, but the callee
> >> +	 * should track its actual progress.
> > 
> > So, if shrinker scans object more than requested, it shoud add up
> > top nr_scanned?
> 
> That sounds fair.
> 
> > opposite case, if shrinker scans less than requested, it should reduce
> > nr_scanned to the value scanned real?
> 
> Unsure. If they can't scan more, the following attempt in the next
> iteration should fail and thus result in SHRINK_STOP?

What should I do if I don't scan anything for some reasons on this iteration
but don't want to stop by SHRINK_STOP because I expect I will scan them
on next iteration? Return 1 on shrinker side? It doesn't make sense.
nr_scanned represents for realy scan value so if shrinker doesn't scan
anything but want to continue the scanning, it can return 0 and VM
should take care of it to prevent infinite loop because shrinker's
expectation can be wrong so it can make the system live-lock.

> 
> > To track the progress is burden for the shrinker users.
> 
> You mean shrinker authors, not users? AFAICS this nr_scanned is opt-in,
> if they don't want to touch it, the default remains nr_to_scan.

I meant shrinker authors which is user for VM shrinker. :-D

Anyway, my point is that shrinker are already racy. IOW, the amount of
objects in a shrinker can be changed between count_object and
scan_object and I'm not sure such micro object tracking based on stale
value will help a lot in every cases.

That means it could be broken interface without guarantee helping
the system as expected.

However, with v1 from Chris, it's low hanging fruit to get without pain
so that's why I wanted to merge v1 rather than v2.

> 
> > Even if a
> > shrinker has a mistake, VM will have big trouble like infinite loop.
> 
> We could fake 0 as 1 or something, at least.

Yes, I think we need it if we want to go this way.