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@xxxxxxxxxxxxx> wrote:
>
> > From: zhouchuyi <zhouchuyi@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
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
