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@xxxxxxxxxxxxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Michal Hocko <mhocko@xxxxxxxx>
>> Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
>> Cc: Hillf Danton <hillf.zj@xxxxxxxxxxxxxxx>
>> Cc: Minchan Kim <minchan@xxxxxxxxxx>
>> Cc: Vlastimil Babka <vbabka@xxxxxxx>
>> Cc: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
>> Cc: Shaohua Li <shli@xxxxxx>
>> Cc: linux-mm@xxxxxxxxx
>> ---
>> 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?
> 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.
> 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.
> IMHO, we need concrete reason to do it but fail to see it at this moment.
>
> Could we just add up more freed object than requested to total_scan
> like you did in first version[1]?
That's a bit different metric, but maybe it doesn't matter. Different
shrinkers are essentially apples and oranges anyway, so improving the
arithmetics can only help to some extent, IMHO.
> [1] lkml.kernel.org/r/<20170812113437.7397-1-chris@xxxxxxxxxxxxxxxxxx>
>
>> + */
>> + unsigned long nr_scanned;
>> +
>> /* current node being shrunk (for NUMA aware shrinkers) */
>> int nid;
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index a1af041930a6..339b8fc95fc9 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -393,14 +393,15 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
>> unsigned long nr_to_scan = min(batch_size, total_scan);
>>
>> shrinkctl->nr_to_scan = nr_to_scan;
>> + shrinkctl->nr_scanned = nr_to_scan;
>> ret = shrinker->scan_objects(shrinker, shrinkctl);
>> if (ret == SHRINK_STOP)
>> break;
>> freed += ret;
>>
>> - count_vm_events(SLABS_SCANNED, nr_to_scan);
>> - total_scan -= nr_to_scan;
>> - scanned += nr_to_scan;
>> + count_vm_events(SLABS_SCANNED, shrinkctl->nr_scanned);
>> + total_scan -= shrinkctl->nr_scanned;
>> + scanned += shrinkctl->nr_scanned;
>
> If we really want to go this way, at least, We need some defense code
> to prevent infinite loop when shrinker doesn't have object any more.
> However, I really want to go with your first version.
>
> Andrew?
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
