From: Vlastimil Babka <vbabka@suse.cz>
To: Minchan Kim <minchan@kernel.org>,
Chris Wilson <chris@chris-wilson.co.uk>
Cc: linux-mm@kvack.org, intel-gfx@lists.freedesktop.org,
Andrew Morton <akpm@linux-foundation.org>,
Michal Hocko <mhocko@suse.com>,
Johannes Weiner <hannes@cmpxchg.org>,
Hillf Danton <hillf.zj@alibaba-inc.com>,
Mel Gorman <mgorman@techsingularity.net>,
Shaohua Li <shli@fb.com>
Subject: Re: [PATCH 1/2] mm: Track actual nr_scanned during shrink_slab()
Date: Thu, 24 Aug 2017 10:00:49 +0200 [thread overview]
Message-ID: <29aae2cd-85a8-f3c4-66e2-4d4f5a2732c1@suse.cz> (raw)
In-Reply-To: <20170824051153.GB13922@bgram>
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@chris-wilson.co.uk>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>> Cc: Hillf Danton <hillf.zj@alibaba-inc.com>
>> Cc: Minchan Kim <minchan@kernel.org>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Mel Gorman <mgorman@techsingularity.net>
>> Cc: Shaohua Li <shli@fb.com>
>> Cc: linux-mm@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?
> 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@chris-wilson.co.uk>
>
>> + */
>> + 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@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>
>
--
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>
next prev parent reply other threads:[~2017-08-24 8:00 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-12 11:34 [PATCH] mm: Reward slab shrinkers that reclaim more than they were asked Chris Wilson
2017-08-15 22:30 ` Andrew Morton
2017-08-15 22:53 ` Chris Wilson
2017-08-22 13:53 ` [PATCH 1/2] mm: Track actual nr_scanned during shrink_slab() Chris Wilson
2017-08-22 13:53 ` [PATCH 2/2] drm/i915: Wire up shrinkctl->nr_scanned Chris Wilson
2017-08-22 22:45 ` Andrew Morton
2017-08-23 14:20 ` Chris Wilson
2017-08-24 5:11 ` [PATCH 1/2] mm: Track actual nr_scanned during shrink_slab() Minchan Kim
2017-08-24 8:00 ` Vlastimil Babka [this message]
2017-08-25 21:41 ` Andrew Morton
2017-08-28 8:09 ` Minchan Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=29aae2cd-85a8-f3c4-66e2-4d4f5a2732c1@suse.cz \
--to=vbabka@suse.cz \
--cc=akpm@linux-foundation.org \
--cc=chris@chris-wilson.co.uk \
--cc=hannes@cmpxchg.org \
--cc=hillf.zj@alibaba-inc.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@techsingularity.net \
--cc=mhocko@suse.com \
--cc=minchan@kernel.org \
--cc=shli@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).