在 2022/8/17 上午11:10, Chuyi Zhou 写道:
在 2022/7/19 下午4:28, Mel Gorman 写道:
On Fri, Jul 15, 2022 at 11:26:01PM +0800, Chuyi Zhou wrote:
??? 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;
The return block indeed may not be aligned. It could simply be a
restart. The changelog could still do with a little clarification but
your patch is still fine.
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);
}
Because valid_page still needs to be set but ensuring valid_page is set
is sufficient.
The pageblock will be marked for skip on the first locking of a lruvec
/* Try get exclusive access under lock */
if (!skip_updated) {
skip_updated = true;
if (test_and_set_skip(cc, page,
low_pfn))
goto isolate_abort;
}
In test_and_set_skip(cc, page, low_pfn), if
!IS_ALIGNED(low_pfn, pageblock_nr_pages) is true, the block would not
get marked skip, furthermore *skip_updated* will be set true which result
second check useless even valid_page has been set.
That will happen regardless of alignment. The intent here was to avoid
multiple scanners trying to isolate the same pageblock. The second
check for update happens at the end of the block
if (low_pfn == end_pfn && (!nr_isolated || cc->rescan)) {
if (valid_page && !skip_updated)
set_pageblock_skip(valid_page);
update_cached_migrate(cc, low_pfn);
}
One way to guarantee a not aligned pageblock get marked is ensuring
valid_page is set and ignoring skip_updated in the second check.
Another way is ignoring IS_ALIGNED(pfn, pageblock_nr_pages) in
test_and_set_skip().
However, both of them may increase the chance of duplicate mark
a pageblock skip.
Tanks.
And it's the second one that requires a valid_page. If valid_page is
available then the pageblock will get marked skip in most cases that
matter. If anything, it can get set prematurely due to "Try get
exclusive access" if the pageblock is not fully scanned but checking if
pageblock should be *cleared* if there was a partial scan would be a
different patch and not even clear that it worth the complexity.
