在 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@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.
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@xxxxxxxxxxxxx>
No need for a newline between Fixes and Signed-off-by. The Signed-off-by
should have your full name, not a username.
