Re: [PATCH 4/3] mm,thp,rmap: handle the normal !PageCompound case first

[Matthew Wilcox <willy@infradead.org>]

On Wed, Nov 09, 2022 at 07:23:08PM -0800, Linus Torvalds wrote:
> On Wed, Nov 9, 2022 at 6:18 PM Hugh Dickins <hughd@google.com> wrote:
> >
> > Commit ("mm,thp,rmap: lock_compound_mapcounts() on THP mapcounts")
> > propagated the "if (compound) {lock} else if (PageCompound) {lock} else
> > {atomic}" pattern throughout; but Linus hated the way that gives primacy
> > to the uncommon case: switch to "if (!PageCompound) {atomic} else if
> > (compound) {lock} else {lock}" throughout.
> 
> Side note, that 'compound' naming is also on my list of "I'm _really_
> not a fan".
> 
> We actually have a completely different meaning for PageCompound()
> than the meaning of 'compound' in the rmap functions, and those
> functions literally mix those meanings if  not on the same line, then
> at least right next to each other.
> 
> What 'rmap' actually means with 'compound' in the add/remove functions
> is basically 'not PAGE_SIZE' as far as I can tell.

Ah.  I've been trying to understand what that 'compound' really means,
and what the difference is to 'PageCompound()' and why we need both.
Thanks!

> One reason I find the "compound" name so horrifying is that it is used
> very much for HUGETLB pages, which I don't think end up ever being
> marked as PageCompund(), and which are - for various historical
> reasons - doubly confusing because they use a "pte_t" to describe
> themselves, even when they are actually using a "pmd_t" or a "pud_t"
> to actually map the page.

HugeTLB pages _are_ marked as Compound.  There's some fairly horrific
code to manually make them compound when they have to be allocated
piecemeal (because they're 1GB and too large for the page allocator).

>    To make things more confusing, some places use PageHeadHuge()
> instead (but the folio version of said test is called
> "folio_test_hugetlb()", just so that nobody could possibly ever accuse
> the HUGETLB code to have consistency).

That one's my fault, but it's a reaction to all the times that I and
others have got confused between PageHuge and PageTransHuge.  I suppose
we could do a big sed s/PageHuge/PageHugeTLB/, but I'm hopeful the
entire hugetlb codebase is either converted to folios or unified with
THP handling.

> I do wish the HUGETLB case didn't use 'pte' for its notion of how
> HUGETLB entries are mapped, but that's literally how HUGETLB is
> designed: it started life as a larger last-level pte.
> 
> It just means that it ends up being very confusing when from a page
> table walk perspective, you're walking a pud or a pmd entry, and then
> you see a 'pte_t' instead.

Yes, one of the long-term things I want to try is making the hugetlb
code use the pmd/pud types like the THP code does.