Re: [PATCH] mm/compactoin: Fix edge case of fast_find_migrateblock()

[Vlastimil Babka <vbabka@suse.cz>]

On 1/23/21 4:43 PM, Wonhyuk Yang wrote:
> In the fast_find_migrateblock(), It iterate freelist to find
> proper pageblock. But there are two edge cases. First, if the page
> we found is equal to cc->migrate_pfn, it is considered that we
> didn't found suitable pageblock. Second, if the loop was terminated
> because order is less than PAGE_ALLOC_COSTLY_ORDER, it could
> be considered that we found suitable one.
> 
> Fixes: 70b44595eafe9 ("mm, compaction: use free lists to quickly locate
> a migration source")
> 
> Signed-off-by: Wonhyuk Yang <vvghjk1234@gmail.com>

Seems correct, although it's quite subtle code and it's been a long time...

Acked-by: Vlastimil Babka <vbabka@suse.cz>

Maybe instead of replacing one magic value of 'pfn' with another
(cc->migrate_pfn with high_pfn) I would just add a "bool found" and use it
appropriately. Would make the function less subtle perhaps.

While reviewing I found more potentially questionable parts, if you're interested:

- if we go through "if (get_pageblock_skip(freepage))... continue;" we are
increasing nr_scanned++; but not checking the limit.

- the "if (list_is_last(freelist, &freepage->lru)) break;" part seems
unneccesary? if we are on the last page and just "continue;" the
list_for_each_entry() iteration should stop anyway.

> ---
>  mm/compaction.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index e5acb9714436..46f49e6b7d1a 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1742,8 +1742,8 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
>  		distance >>= 2;
>  	high_pfn = pageblock_start_pfn(cc->migrate_pfn + distance);
>  
> -	for (order = cc->order - 1;
> -	     order >= PAGE_ALLOC_COSTLY_ORDER && pfn == cc->migrate_pfn && nr_scanned < limit;
> +	for (order = cc->order - 1, pfn = high_pfn;
> +	     order >= PAGE_ALLOC_COSTLY_ORDER && pfn == high_pfn && nr_scanned < limit;
>  	     order--) {
>  		struct free_area *area = &cc->zone->free_area[order];
>  		struct list_head *freelist;
> @@ -1785,7 +1785,6 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
>  			}
>  
>  			if (nr_scanned >= limit) {
> -				cc->fast_search_fail++;
>  				move_freelist_tail(freelist, freepage);
>  				break;
>  			}
> @@ -1799,9 +1798,10 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
>  	 * If fast scanning failed then use a cached entry for a page block
>  	 * that had free pages as the basis for starting a linear scan.
>  	 */
> -	if (pfn == cc->migrate_pfn)
> +	if (pfn == high_pfn) {
> +		cc->fast_search_fail++;
>  		pfn = reinit_migrate_pfn(cc);
> -
> +	}
>  	return pfn;
>  }
>  
>