Date:   Fri, 15 Jan 2021 20:51:18 +0530
From:   Charan Teja Kalla <charante@...eaurora.org>
To:     Vlastimil Babka <vbabka@...e.cz>, akpm@...ux-foundation.org,
        mhocko@...e.com, khalid.aziz@...cle.com, ngupta@...ingupta.dev,
        vinmenon@...eaurora.org
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/compaction: return proper state in
 should_proactive_compact_node

Thank you Vlastimil!!

On 1/15/2021 6:15 PM, Vlastimil Babka wrote:
> On 1/13/21 3:03 PM, Charan Teja Reddy wrote:
>> should_proactive_compact_node() returns true when sum of the
>> fragmentation score of all the zones in the node is greater than the
>> wmark_high of compaction which then triggers the proactive compaction
>> that operates on the individual zones of the node. But proactive
>> compaction runs on the zone only when the fragmentation score of the
>> zone is greater than wmark_low(=wmark_high - 10).
>>
>> This means that the sum of the fragmentation scores of all the zones can
>> exceed the wmark_high but individual zone scores can still be less than
>> the wmark_low which makes the unnecessary trigger of the proactive
>> compaction only to return doing nothing.
>>
>> Another issue with the return of proactive compaction with out even
>> trying is its deferral. It is simply deferred for 1 <<
>> COMPACT_MAX_DEFER_SHIFT if the scores across the proactive compaction is
>> same, thinking that compaction didn't make any progress but in reality
>> it didn't even try. With the delay between successive retries for
>> proactive compaction is 500msec, it can result into the deferral for
>> ~30sec with out even trying the proactive compaction.
>>
>> Test scenario is that: compaction_proactiveness=50 thus the wmark_low =
>> 50 and wmark_high = 60. System have 2 zones(Normal and Movable) with
>> sizes 5GB and 6GB respectively. After opening some apps on the android,
>> the fragmentation scores of these zones are 47 and 49 respectively.
>> Since the sum of these fragmentation scores are above the wmark_high
>> which triggers the proactive compaction and there since the individual
>> zone scores are below wmark_low, it returns without trying the
>> compaction. As a result the fragmentation scores of the zones are still
>> 47 and 49 which makes the existing logic to defer the compaction
>> thinking that noprogress is made across the compaction.
>>
>> So, run the proactive compaction on the node zones only when atleast one
>> of the zones fragmentation score is greater than wmark_low. This avoids
>> the unnecessary deferral and retries of the compaction.
>>
>> Signed-off-by: Charan Teja Reddy <charante@...eaurora.org>
> 
> Good catch about the problem, but I wonder if the solution could be better.
> 
> fragmentation_score_node() is a weighted average of scores of all zones, that's
> why fragmentation_score_zone() adjusts the score by zone_present/node_present.
> 
> But when considering an individual zone in __compact_finished(), we shouldn't be
> using fragmentation_score_zone() with the adjustment. We are not calculating the
> weighted average for the whole node there, so it doesn't make sense to do the
> adjustment by size. So if it simply took extfrag_for_order(...) as the score, it
> should work as expected. In your example above, the score of each zone would be
> above 60. If the weighted average is above wmark_high, then individual score
> (not adjusted) of at least one zone has to be above wmark_high, and the extra
> check using max() is not necessary.
> 
> So I would split fragmentation_score_zone() to e.g. fragmentation_score_zone()
> and fragmentation_score_zone_weighted() and call the latter only from
> fragmentation_score_node(), and not from __compact_finished().

This suggestion looks good and much cleaner. Will raise V2 in couple of
days. Thanks.

> 
> Vlastimil
> 
>> ---
>>  mm/compaction.c | 27 +++++++++++++++++++++++++--
>>  1 file changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index e5acb97..f7a772a 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -1964,6 +1964,26 @@ static unsigned int fragmentation_score_node(pg_data_t *pgdat)
>>  	return score;
>>  }
>>  
>> +/*
>> + * Returns the maximum of fragmentation scores of zones in a node. This is
>> + * used in taking the decission of whether to trigger the proactive compaction
>> + * on the zones of this node.
>> + */
>> +static unsigned int fragmentation_score_node_zones_max(pg_data_t *pgdat)
>> +{
>> +	int zoneid;
>> +	unsigned int max = 0;
>> +
>> +	for (zoneid = 0; zoneid < MAX_NR_ZONES; zoneid++) {
>> +		struct zone *zone;
>> +
>> +		zone = &pgdat->node_zones[zoneid];
>> +		max = max_t(unsigned int, fragmentation_score_zone(zone), max);
>> +	}
>> +
>> +	return max;
>> +}
>> +
>>  static unsigned int fragmentation_score_wmark(pg_data_t *pgdat, bool low)
>>  {
>>  	unsigned int wmark_low;
>> @@ -1979,13 +1999,16 @@ static unsigned int fragmentation_score_wmark(pg_data_t *pgdat, bool low)
>>  
>>  static bool should_proactive_compact_node(pg_data_t *pgdat)
>>  {
>> -	int wmark_high;
>> +	int wmark_low, wmark_high;
>>  
>>  	if (!sysctl_compaction_proactiveness || kswapd_is_running(pgdat))
>>  		return false;
>>  
>>  	wmark_high = fragmentation_score_wmark(pgdat, false);
>> -	return fragmentation_score_node(pgdat) > wmark_high;
>> +	wmark_low = fragmentation_score_wmark(pgdat, true);
>> +
>> +	return fragmentation_score_node(pgdat) > wmark_high &&
>> +		fragmentation_score_node_zones_max(pgdat) > wmark_low;
>>  }
>>  
>>  static enum compact_result __compact_finished(struct compact_control *cc)
>>
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project

Powered by blists - more mailing lists