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

[Mel Gorman <mgorman@techsingularity.net>]

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.

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?

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.

-- 
Mel Gorman
SUSE Labs