[PATCH] mm/compaction: return proper state in should_proactive_compact_node

[PATCH] mm/compaction: return proper state in should_proactive_compact_node

[Charan Teja Reddy]

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@codeaurora.org>
---
 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)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] mm/compaction: return proper state in should_proactive_compact_node

[Andrew Morton]

On Wed, 13 Jan 2021 19:33:06 +0530 Charan Teja Reddy <charante@codeaurora.org> 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.
> 
> --- 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);

Both args are unsigned int, so I think the max_t is unnecessary?

--- a/mm/compaction.c~mm-compaction-return-proper-state-in-should_proactive_compact_node-fix
+++ a/mm/compaction.c
@@ -1975,7 +1975,7 @@ static unsigned int fragmentation_score_
 		struct zone *zone;
 
 		zone = &pgdat->node_zones[zoneid];
-		max = max_t(unsigned int, fragmentation_score_zone(zone), max);
+		max = max(fragmentation_score_zone(zone), max);
 	}
 
 	return max;
_

Re: [PATCH] mm/compaction: return proper state in should_proactive_compact_node

[Vlastimil Babka]

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@codeaurora.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().

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)
> 

Re: [PATCH] mm/compaction: return proper state in should_proactive_compact_node

[Charan Teja Kalla]

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@codeaurora.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