Re: [PATCH v3] mm/compaction: fix set skip in fast_find_migrateblock

[Chuyi Zhou <zhouchuyi@bytedance.com>]

在 2022/7/14 下午7:50, Mel Gorman 写道:
> On Wed, Jul 13, 2022 at 08:28:14AM -0700, Andrew Morton wrote:
>> (cc Mel)
>>
>> On Wed, 13 Jul 2022 14:20:09 +0800 Chuyi Zhou <zhouchuyi@bytedance.com> wrote:
>>
>>> From: zhouchuyi <zhouchuyi@bytedance.com>
>>>
>>> When we successfully find a pageblock in fast_find_migrateblock(),
>>> the block will be set skip-flag through set_pageblock_skip(). However,
>>> when entering isolate_migratepages_block(), the whole pageblock will
>>> be skipped due to the branch
>>> 'if (!valid_page && IS_ALIGNED(low_pfn, pageblock_nr_pages))'.
>>> Eventually we will goto isolate_abort and isolate nothing. That cause
>>> fast_find_migrateblock useless.
>>>
> 
> It's not very clear *why* this failed from the changelog because
> superficially !valid_page will be true for the first pageblock and there
> is a reasonable expectation it will be aligned. Is the following accurate
> based on your analysis?
> 
> 	However, when entering isolate_migratepages_block(), the first
> 	pageblock will be skipped in the branch 'if (!valid_page &&
> 	IS_ALIGNED(low_pfn, pageblock_nr_pages))' as isolation_suitable
> 	returns true due to the skip bit set by fast_find_migrateblock().
> 
> If so, please update the changelog as a reviewer handling backports may
> wonder what exactly is wrong with that branch.
> 
Hi Mel, thanks for your review.

If fast scanning failed, the return block may not be aligned, because we 
get pfn from *free_pfn*. When fast-find success, the return value *pfn* 
is get from pageblock_start_pfn, and it will be passed to 
isolate_migratepages_block as low_pfn. I think normally the value get 
from pageblock_start_pfn should be aligned with pageblock_nr_pages. I 
have used printk test it. Maby I miss something important?
     pfn = pageblock_start_pfn(free_pfn);
     ...
     found_block = true;
     set_pageblock_skip(freepage);
     break;
> Second, what guarantees a block returned by fast_find that is not
> aligned gets marked skipped after it is scanned? The set_pageblock_skip
> is only called when there is a valid page and it may not be set if
> !IS_ALIGNED(low_pfn). Is something like this untested hunk also necessary?
> 
You are right, we do need some machenism to ensure mark skipped after
scanned a !IS_ALIGNED block from fast_find. However, I think the 
following code may not working. Because *skip_updated* has been reset:
     if (!skip_updated) {
         skip_updated = true;
         if (test_and_set_skip(cc, page, low_pfn))
             goto isolate_abort;
     }
Why not ignore skip_updated after scanned a block, just like this:

diff --git a/mm/compaction.c b/mm/compaction.c
index 962d05d1e187..1c388c45f127 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1180,7 +1180,7 @@ isolate_migratepages_block(struct compact_control 
*cc, unsigned long low_pfn,
          * rescanned twice in a row.
          */
         if (low_pfn == end_pfn && (!nr_isolated || cc->rescan)) {
-               if (valid_page && !skip_updated)
+               if (valid_page)
                         set_pageblock_skip(valid_page);
                 update_cached_migrate(cc, low_pfn);
         }

> diff --git a/mm/compaction.c b/mm/compaction.c
> index 1f89b969c12b..112346b2f716 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -888,8 +888,9 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>   		 * COMPACT_CLUSTER_MAX at a time so the second call must
>   		 * not falsely conclude that the block should be skipped.
>   		 */
> -		if (!valid_page && IS_ALIGNED(low_pfn, pageblock_nr_pages)) {
> -			if (!isolation_suitable(cc, page)) {
> +		if (!valid_page) {
> +			if (IS_ALIGNED(low_pfn, pageblock_nr_pages) &&
> +			    !isolation_suitable(cc, page)) {
>   				low_pfn = end_pfn;
>   				page = NULL;
>   				goto isolate_abort;
> 
>>> In this Patch, when we find a suitable pageblock in fast_find_
>>> migrateblock, we do noting but let isolate_migratepages_block
>>> to set skip flag to the pageblock after scan it. Normally,
>>> we would isolate some pages from the fast-find block.
>>>
>>> I use mmtest/thpscale-madvhugepage test it. Here is the result:
>>>                              baseline               patch
>>> Amean     fault-both-1      1331.66 (   0.00%)     1261.04 *   5.30%*
>>> Amean     fault-both-3      1383.95 (   0.00%)     1191.69 *  13.89%*
>>> Amean     fault-both-5      1568.13 (   0.00%)     1445.20 *   7.84%*
>>> Amean     fault-both-7      1819.62 (   0.00%)     1555.13 *  14.54%*
>>> Amean     fault-both-12     1106.96 (   0.00%)     1149.43 *  -3.84%*
>>> Amean     fault-both-18     2196.93 (   0.00%)     1875.77 *  14.62%*
>>> Amean     fault-both-24     2642.69 (   0.00%)     2671.21 *  -1.08%*
>>> Amean     fault-both-30     2901.89 (   0.00%)     2857.32 *   1.54%*
>>> Amean     fault-both-32     3747.00 (   0.00%)     3479.23 *   7.15%*
>>>
>>> Fixes: 70b44595eafe9 ("mm, compaction: use free lists to quickly locate a migration source")
>>>
>>> Signed-off-by: zhouchuyi <zhouchuyi@bytedance.com>
> 
> No need for a newline between Fixes and Signed-off-by. The Signed-off-by
> should have your full name, not a username.
>