Re: [PATCH 1/5] mm: vmscan: shrink all slab objects if tight on memory

[Vladimir Davydov <vdavydov@parallels.com>]

On 01/16/2014 02:53 AM, Andrew Morton wrote:
> On Wed, 15 Jan 2014 19:55:11 +0400 Vladimir Davydov <vdavydov@parallels.com> wrote:
>
>>> We could avoid the "scan 32 then scan just 1" issue with something like
>>>
>>> 	if (total_scan > batch_size)
>>> 		total_scan %= batch_size;
>>>
>>> before the loop.  But I expect the effects of that will be unmeasurable
>>> - on average the number of objects which are scanned in the final pass
>>> of the loop will be batch_size/2, yes?  That's still a decent amount.
>> Let me try to summarize. We want to scan batch_size objects in one pass,
>> not more (to keep latency low) and not less (to avoid cpu cache
>> pollution due to too frequent calls); if the calculated value of
>> nr_to_scan is less than the batch_size we should accumulate it in
>> nr_deferred instead of calling ->scan() and add nr_deferred to
>> nr_to_scan on the next pass, i.e. in pseudo-code:
>>
>>     /* calculate current nr_to_scan */
>>     max_pass = shrinker->count();
>>     delta = max_pass * nr_user_pages_scanned / nr_user_pages;
>>
>>     /* add nr_deferred */
>>     total_scan = delta + nr_deferred;
>>
>>     while (total_scan >= batch_size) {
>>         shrinker->scan(batch_size);
>>         total_scan -= batch_size;
>>     }
>>
>>     /* save the remainder to nr_deferred  */
>>     nr_deferred = total_scan;
>>
>> That would work, but if max_pass is < batch_size, it would not scan the
>> objects immediately even if prio is high (we want to scan all objects).
> Yes, that's a problem.
>
>> For example, dropping caches would not work on the first attempt - the
>> user would have to call it batch_size / max_pass times.
> And we do want drop_caches to work immediately.
>
>> This could be
>> fixed by making the code proceed to ->scan() not only if total_scan is
>>> = batch_size, but also if max_pass is < batch_size and total_scan is >=
>> max_pass, i.e.
>>
>>     while (total_scan >= batch_size ||
>>             (max_pass < batch_size && total_scan >= max_pass)) ...
>>
>> which is equivalent to
>>
>>     while (total_scan >= batch_size ||
>>                 total_scan >= max_pass) ...
>>
>> The latter is the loop condition from the current patch, i.e. this patch
>> would make the trick if shrink_slab() followed the pseudo-code above. In
>> real life, it does not actually - we have to bias total_scan before the
>> while loop in order to avoid dropping fs meta caches on light memory
>> pressure due to a large number being built in nr_deferred:
>>
>>     if (delta < max_pass / 4)
>>         total_scan = min(total_scan, max_pass / 2);
> Oh, is that what's it's for.  Where did you discover this gem?

>From 3567b59aa80ac4417002bf58e35dce5c777d4164 Mon Sep 17 00:00:00 2001
From: Dave Chinner <dchinner@redhat.com>
Date: Fri, 8 Jul 2011 14:14:36 +1000
Subject: [PATCH] vmscan: reduce wind up shrinker->nr when shrinker can't do
 work

>>     while (total_scan >= batch_size) ...
>>
>> With this biasing, it is impossible to achieve the ideal behavior I've
>> described above, because we will never accumulate max_pass objects in
>> nr_deferred if memory pressure is low. So, if applied to the real code,
>> this patch takes on a slightly different sense, which I tried to reflect
>> in the comment to the code: it will call ->scan() with nr_to_scan <
>> batch_size only if:
>>
>> 1) max_pass < batch_size && total_scan >= max_pass
>>
>> and
>>
>> 2) we're tight on memory, i.e. the current delta is high (otherwise
>> total_scan will be biased as max_pass / 2 and condition 1 won't be
>> satisfied).
> (is max_pass misnamed?)

Yes, the name is misleading. I guess, it should be called freeable_cnt,
because we actually scan up to 2*max_pass objects in one pass.

>> >From our discussion it seems condition 2 is not necessary at all, but it
>> follows directly from the biasing rule. So I propose to tweak the
>> biasing a bit so that total_scan won't be lowered < batch_size:
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index eea668d..78ddd5e 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -267,7 +267,7 @@ shrink_slab_node(struct shrink_control *shrinkctl,
>> struct shrinker *shrinker,
>>       * a large delta change is calculated directly.
>>       */
>>      if (delta < max_pass / 4)
>> -        total_scan = min(total_scan, max_pass / 2);
>> +        total_scan = min(total_scan, max(max_pass / 2, batch_size));
>>  
>>      /*
>>       * Avoid risking looping forever due to too large nr value:
>> @@ -281,7 +281,7 @@ shrink_slab_node(struct shrink_control *shrinkctl,
>> struct shrinker *shrinker,
>>                  nr_pages_scanned, lru_pages,
>>                  max_pass, delta, total_scan);
>>  
>> -    while (total_scan >= batch_size) {
>> +    while (total_scan >= batch_size || total_scan >= max_pass) {
>>          unsigned long ret;
>>  
>>          shrinkctl->nr_to_scan = batch_size;
>>
>> The first hunk guarantees that total_scan will always accumulate at
>> least batch_size objects no matter how small max_pass is. That means
>> that when max_pass is < batch_size we will eventually get >= max_pass
>> objects to scan and shrink the slab to 0 as we need. What do you think
>> about that?
> I'm a bit lost :(

I'll try to clean up shrink_slab_node() and resend the patch then.

Thanks.