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

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

[Wonhyuk Yang]

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>
---
 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;
 }
 
-- 
2.25.1

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

[Vlastimil Babka]

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;
>  }
>  
> 

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

[Wonhyuk Yang]

On Tue, Jan 26, 2021 at 1:49 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> 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.

I agree. Using that bool variable will look good.

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

Yes, you've got a point. I think nr_scanned checking should be first.

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

Yes It seems unnecessary. only if the skipped block was reordered, it
would be required.

Then I will reflect your opinion and send a new version.