On 7/19/22 10:28, Mel Gorman wrote:
> 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;
> }
>
> That will happen regardless of alignment.
I'm not sure about that as test_and_set_skip() has "if
(!pageblock_aligned(pfn)) return false". So I think it only takes for the
first pfn in pageblock to not be PageLRU and we don't try to get the
exclusive access for it, and the following pfn's will not be aligned, and
thus we never set_pageblock_skip() through this path. If we have nr_isolated
> 0 and not cc->rescan, we might not set_pageblock_skip() through the hunk
above neither.
I've been checking this area because we have some reports [1] for upstream
6.1 causing some long loops in khugepaged pegging 100% CPU in compaction.
Tracepoint data suggests we keep (successfully) isolating migratepages over
and over through fast_find_migrateblock() (the pfn ranges are not linearly
increasing but there's a cycle probably as we rotate the freelist), but are
failing to migrate them (for some reason). Between 6.0 and 6.1 there's this
patch as commit 7efc3b726103 so I strongly suspect it's the root cause and
will be providing a kernel with revert to in the bug to confirm.
Consider this an early heads up :)
[1] https://bugzilla.suse.com/show_bug.cgi?id=1206848
> 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);
> }
>
> 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.
>
