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@xxxxxxxxx>
Seems correct, although it's quite subtle code and it's been a long time...
Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
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;
> }
>
>
