Re: [RFC PATCH v2 1/5] mm: Identify compound pages sooner in isolate_migratepages_block

[Alexander Duyck]

On Wed, Aug 19, 2020 at 4:43 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Tue, Aug 18, 2020 at 09:27:05PM -0700, Alexander Duyck wrote:
> > +             /*
> > +              * Page is compound. We know the order before we know if it is
> > +              * on the LRU so we cannot assume it is THP. However since the
> > +              * page will have the LRU validated shortly we can use the value
> > +              * to skip over this page for now or validate the LRU is set and
> > +              * then isolate the entire compound page if we are isolating to
> > +              * generate a CMA page.
> > +              */
> > +             if (PageCompound(page)) {
> > +                     const unsigned int order = compound_order(page);
> > +
> > +                     if (likely(order < MAX_ORDER))
> > +                             low_pfn += (1UL << order) - 1;
>
> Hmm.  You're checking for PageCompound but then skipping 1UL << order.
> That only works if PageHead.  If instead this is PageCompound because
> it's PageTail, you need to do something like:
>
>                                 low_pfn |= (1UL << order) - 1;
>
> which will move you to the end of the page you're in the middle of.

Can you successfully call get_page_unless_zero in a tail page? I
thought their reference count was 0? There is a get_page_unless_zero
call before the PageCompound check, so I don't think we can get a tail
page.

> If PageTail can't actually happen here, then it's better to check for
> PageHead explicitly and WARN_ON if you get a PageTail (eg a page was
> combined into a compound page after you processed the earlier head page).
>
> Is it possible the page you've found is hugetlbfs?  Those can have orders
> larger than MAX_ORDER.

So in theory we only need to jump pageblock_order. However there are
some architectures where that is not a fixed constant and so it would
have some additional overhead if I am not mistaken. In addition we
should have been only provided a pageblock if i am not mistaken so the
check further down that prevents low_pfn from passing end_pfn should
reset to the correct value.