Thanks Vlasitmil!! On 1/18/2021 6:07 PM, Vlastimil Babka wrote: > On 1/18/21 1:20 PM, Charan Teja Reddy wrote: >> should_proactive_compact_node() returns true when sum of the >> weighted 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 its weighted >> fragmentation score is greater than wmark_low(=wmark_high - 10). >> >> This means that the sum of the weighted fragmentation scores of all the >> zones can exceed the wmark_high but individual weighted fragmentation >> zone scores can still be less than wmark_low which makes the unnecessary >> trigger of the proactive compaction only to return doing nothing. >> >> 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 weighted 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 zones weighted fragmentation scores are below wmark_low, it >> returns without trying the proactive compaction. As a result the >> weighted 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. >> >> Fix this by checking just zone fragmentation score, not the weighted, in >> __compact_finished() and use the zones weighted fragmentation score in >> fragmentation_score_node(). In the test case above, If the weighted >> average of is above wmark_high, then individual score (not adjusted) of >> atleast one zone has to be above wmark_high. Thus it avoids the >> unnecessary trigger and deferrals of the proactive compaction. >> >> Fix-suggested-by: Vlastimil Babka <vbabka@xxxxxxx> >> Signed-off-by: Charan Teja Reddy <charante@xxxxxxxxxxxxxx> > > Acked-by: Vlastimil Babka <vbabka@xxxxxxx> > > But I would move fragmentation_score_zone() above > fragmentation_score_zone_weighted(), so fragmentation_score_zone_weighted() can > call fragmentation_score_zone() instead of having two places with > extfrag_for_order(...). Yes, this suggestion makes the code cleaner. I will raise V3 for this. Thanks. > > Thanks. > >> --- >> >> Changes in V2: Addressed comments from vlastimil >> >> Changes in V1: https://lore.kernel.org/patchwork/patch/1364646/ >> >> mm/compaction.c | 19 ++++++++++++++----- >> 1 file changed, 14 insertions(+), 5 deletions(-) >> >> diff --git a/mm/compaction.c b/mm/compaction.c >> index e5acb97..1b98427 100644 >> --- a/mm/compaction.c >> +++ b/mm/compaction.c >> @@ -1924,16 +1924,16 @@ static bool kswapd_is_running(pg_data_t *pgdat) >> } >> >> /* >> - * A zone's fragmentation score is the external fragmentation wrt to the >> - * COMPACTION_HPAGE_ORDER scaled by the zone's size. It returns a value >> - * in the range [0, 100]. >> + * A weighted zone's fragmentation score is the external fragmentation >> + * wrt to the COMPACTION_HPAGE_ORDER scaled by the zone's size. It >> + * returns a value in the range [0, 100]. >> * >> * The scaling factor ensures that proactive compaction focuses on larger >> * zones like ZONE_NORMAL, rather than smaller, specialized zones like >> * ZONE_DMA32. For smaller zones, the score value remains close to zero, >> * and thus never exceeds the high threshold for proactive compaction. >> */ >> -static unsigned int fragmentation_score_zone(struct zone *zone) >> +static unsigned int fragmentation_score_zone_weighted(struct zone *zone) >> { >> unsigned long score; >> >> @@ -1943,6 +1943,15 @@ static unsigned int fragmentation_score_zone(struct zone *zone) >> } >> >> /* >> + * A zone's fragmentation score is the external fragmentation wrt to the >> + * COMPACTION_HPAGE_ORDER. It returns a value in the range [0, 100]. >> + */ >> +static unsigned int fragmentation_score_zone(struct zone *zone) >> +{ >> + return extfrag_for_order(zone, COMPACTION_HPAGE_ORDER); >> +} >> + >> +/* >> * The per-node proactive (background) compaction process is started by its >> * corresponding kcompactd thread when the node's fragmentation score >> * exceeds the high threshold. The compaction process remains active till >> @@ -1958,7 +1967,7 @@ static unsigned int fragmentation_score_node(pg_data_t *pgdat) >> struct zone *zone; >> >> zone = &pgdat->node_zones[zoneid]; >> - score += fragmentation_score_zone(zone); >> + score += fragmentation_score_zone_weighted(zone); >> } >> >> return score; >> > -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
