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

[Mel Gorman <mgorman@techsingularity.net>]

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@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.
> > 
> 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. 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.

-- 
Mel Gorman
SUSE Labs