diff mbox series

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

Message ID fca2f694-2098-b0ef-d4e-f1d8b94d318c@google.com
State New
Headers
Series mm,huge,rmap: unify and speed up compound mapcounts |

Commit Message

Hugh Dickins Nov. 10, 2022, 2:18 a.m. UTC 
  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.  Linus has a bigger idea
for how to improve it all, but here just make that rearrangement.

Signed-off-by: Hugh Dickins <hughd@google.com>
---
 mm/rmap.c | 54 +++++++++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

Comments

Linus Torvalds Nov. 10, 2022, 3:23 a.m. UTC | #1 
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.

So if I get the energy to do the rmap counts, I will *also* be
renaming that horrible thing completely.

In fact, I'd be inclined to just pass in the actual page size
(possibly a page shift order), which some of the functions want
anyway, and which would be a lot clearer than the horrid "compound"
name.

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.

So a HUGETLB entry really is (for historical reasons) designed to look
like a single last-level pte_t entry, but from an rmap perspective it
is explicitly *not* made to look like that at all, completely
violating the HUGETLB design.

So the rmap code has several really REALLY confusing cases:

 - the common one: just a page mapped at a *real* pte_t level.

   To make that more confusing, it can actually be a single-page
_part_ of a compound page in the PageCompound() sense, but the rmap
'compound' argument will *not* be set, because from a *mmap*
standpoint it's mapped as a single page.

   This is generally recognized by the rmap code by 'compound' being zero.

 - a HUGETLB mapping, which uses '->pte' in the page walking (please
kill me now) and is *not* using a PageCompund() page, but 'compound'
is still set, because from a *mapping* standpoint it's not a final
pte_t entry (buit from a MM design standpoint it _was_ supposed to be
designed like a single page).

   This is randomly recognized by looking at the vma flags (using
"is_vm_hugetlb_page(vma)") or just inherent in the code itself (ie the
'hugetlb()' functions are only called by code that has tested this
situation one way or another)

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

    You'd think that PageHeadHuge() is one of the functions that
checks the page flag bits. You'd be wrong. It's very very special.

 - an *actual* PageCompound() page, mapped as such as a THP page (ie
mapped by a pmd, not a pte).

   This may be the same page size as a HUGETLB mapping (and often is),
but it's a completely different case in every single way.

   But like the HUGETLB case, the 'compound' argument will be set, and
now it's actually a compound page (but hey, so could the single page
mapping case be too).

   Unlike the HUGETLB case, the page walker does not use ->pte for
this, and some of the walkers will very much use that, ie
folio_referenced_one() will do

                if (pvmw.pte) {

   to distinguish the "natively mapped PageCompound()" case (no pte)
from the "map a single page" or from the HUGETLB case (yes pte).

There may be more cases than those three, and I may have confused
myself and gotten some of the details wrong, but I did want to write
the above diatribe out to

 (a) see if somebody corrects me for any of the cases I enumerated

 (b) see if somebody can point to yet another odd case

 (c) see if somebody has suggestions for better and more obvious names
for that 'compound' argument in the rmap code

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.

An example of that confusion is visible in try_to_unmap_one(), which
can be called with a HUGEPTE page (well, folio), and that does

        while (page_vma_mapped_walk(&pvmw)) {

to find the rmap entries, but it can't do that

                if (pvmw.pte) {

test to see what mapping it's walking (since both regular pages and
HUGETLB pages use that), so then it just keeps testing what kind of
page that was passed in.

Which really smells very odd to me, but apparently it works,
presumably because unlike THP there can be no splitting.  But it's a
case where the whole "was it a regular page or a HUGETLB page" is
really really confusing/

And mm/hugetlb.c (and places like mm/pagewalk.c too) has a fair number
of random casts as a result of this "it's not really a pte_t, but it's
designed to look like one" thing.

This all really is understandable from a historical context, and from
HUGETLB really being just kind of designed to be a bigger page (not a
collection of small pages that can be mapped as a bigger entity), but
it really does mean that 'rmap' calling those HUGETLB pages 'compound'
is conceptually very very wrong.

Oh well. That whole HUGETLB model isn't getting fixed, but I think the
naming confusion about 'compound' *can* be fixed fairly easily, and we
could try to at least avoid having 'compound' and 'PageCompound()'
mean totally different things in the same function.

I'm not going to do any of this cleanup now, but I wanted to at least
voice my concerns. Maybe I'll get around to actually trying to clarify
the code later.

Because this was all stuff that was *very* confusing when I did the
rmap simplification in that (now completely rewritten to explicitly
_not_ touch rmap at all) original version of the delayed rmap patch
series.

                 Linus
Hugh Dickins Nov. 10, 2022, 4:21 a.m. UTC | #2 
On Wed, 9 Nov 2022, 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.
> 
> So if I get the energy to do the rmap counts,

See other mail, I got some zest to try your idea on the counts.

> I will *also* be renaming that horrible thing completely.

But I don't suppose I'll spend time on that part, I don't really
see the problem.  "compound" might be better named, say, "large_rmap"
(I'd have said "pmd_mapped" or "pmd_rmap", but you raise the spectre
of hugetlb below, and powerpc as usual does hugetlb very differently),
but compound seems okay to me, and consistent with usage elsewhere.

> 
> In fact, I'd be inclined to just pass in the actual page size
> (possibly a page shift order), which some of the functions want
> anyway, and which would be a lot clearer than the horrid "compound"
> name.

But yes, I think that would be an improvement; yet you might find a
reason why so often we don't do that - there's often an awkward
BUILD_BUG when you build without CONFIG_TRANSPARENT_HUGEPAGE=y.
And much as I've often wanted to remove it, it does give some
assurance that we're not bloating THP-disabled configs.  Maybe the
steady growth of compound_nr() usage gets around that better now
(or will you be renaming that too ?-)

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

hugetlb pages are always PageCompound.  Shoot me if they're not.

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

Yes, I wish we would undo that hugetlb deception: it would probably
be much more (un)doable, were it not for powerpc (and ia64 iirc).

> 
> So a HUGETLB entry really is (for historical reasons) designed to look
> like a single last-level pte_t entry, but from an rmap perspective it
> is explicitly *not* made to look like that at all, completely
> violating the HUGETLB design.
> 
> So the rmap code has several really REALLY confusing cases:
> 
>  - the common one: just a page mapped at a *real* pte_t level.
> 
>    To make that more confusing, it can actually be a single-page
> _part_ of a compound page in the PageCompound() sense, but the rmap
> 'compound' argument will *not* be set, because from a *mmap*
> standpoint it's mapped as a single page.

Yes.  Most pages are unambiguous, but when a PageHead page arrives
at page_add/remove_rmap(), we have to do different things, according
to whether it's mapped with a large or a small entry.

But I'm going away at this point, you write much faster than I can
read and understand and respond.  I'm responding in part to "fix"
my stupid typo on Johannes's address.

Hugh

> 
>    This is generally recognized by the rmap code by 'compound' being zero.
> 
>  - a HUGETLB mapping, which uses '->pte' in the page walking (please
> kill me now) and is *not* using a PageCompund() page, but 'compound'
> is still set, because from a *mapping* standpoint it's not a final
> pte_t entry (buit from a MM design standpoint it _was_ supposed to be
> designed like a single page).
> 
>    This is randomly recognized by looking at the vma flags (using
> "is_vm_hugetlb_page(vma)") or just inherent in the code itself (ie the
> 'hugetlb()' functions are only called by code that has tested this
> situation one way or another)
> 
>    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).
> 
>     You'd think that PageHeadHuge() is one of the functions that
> checks the page flag bits. You'd be wrong. It's very very special.
> 
>  - an *actual* PageCompound() page, mapped as such as a THP page (ie
> mapped by a pmd, not a pte).
> 
>    This may be the same page size as a HUGETLB mapping (and often is),
> but it's a completely different case in every single way.
> 
>    But like the HUGETLB case, the 'compound' argument will be set, and
> now it's actually a compound page (but hey, so could the single page
> mapping case be too).
> 
>    Unlike the HUGETLB case, the page walker does not use ->pte for
> this, and some of the walkers will very much use that, ie
> folio_referenced_one() will do
> 
>                 if (pvmw.pte) {
> 
>    to distinguish the "natively mapped PageCompound()" case (no pte)
> from the "map a single page" or from the HUGETLB case (yes pte).
> 
> There may be more cases than those three, and I may have confused
> myself and gotten some of the details wrong, but I did want to write
> the above diatribe out to
> 
>  (a) see if somebody corrects me for any of the cases I enumerated
> 
>  (b) see if somebody can point to yet another odd case
> 
>  (c) see if somebody has suggestions for better and more obvious names
> for that 'compound' argument in the rmap code
> 
> 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.
> 
> An example of that confusion is visible in try_to_unmap_one(), which
> can be called with a HUGEPTE page (well, folio), and that does
> 
>         while (page_vma_mapped_walk(&pvmw)) {
> 
> to find the rmap entries, but it can't do that
> 
>                 if (pvmw.pte) {
> 
> test to see what mapping it's walking (since both regular pages and
> HUGETLB pages use that), so then it just keeps testing what kind of
> page that was passed in.
> 
> Which really smells very odd to me, but apparently it works,
> presumably because unlike THP there can be no splitting.  But it's a
> case where the whole "was it a regular page or a HUGETLB page" is
> really really confusing/
> 
> And mm/hugetlb.c (and places like mm/pagewalk.c too) has a fair number
> of random casts as a result of this "it's not really a pte_t, but it's
> designed to look like one" thing.
> 
> This all really is understandable from a historical context, and from
> HUGETLB really being just kind of designed to be a bigger page (not a
> collection of small pages that can be mapped as a bigger entity), but
> it really does mean that 'rmap' calling those HUGETLB pages 'compound'
> is conceptually very very wrong.
> 
> Oh well. That whole HUGETLB model isn't getting fixed, but I think the
> naming confusion about 'compound' *can* be fixed fairly easily, and we
> could try to at least avoid having 'compound' and 'PageCompound()'
> mean totally different things in the same function.
> 
> I'm not going to do any of this cleanup now, but I wanted to at least
> voice my concerns. Maybe I'll get around to actually trying to clarify
> the code later.
> 
> Because this was all stuff that was *very* confusing when I did the
> rmap simplification in that (now completely rewritten to explicitly
> _not_ touch rmap at all) original version of the delayed rmap patch
> series.
> 
>                  Linus
Matthew Wilcox Nov. 10, 2022, 4:31 p.m. UTC | #3 
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.
Linus Torvalds Nov. 10, 2022, 4:58 p.m. UTC | #4 
On Thu, Nov 10, 2022 at 8:31 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> Ah.  I've been trying to understand what that 'compound' really means,
> and what the difference is to 'PageCompound()' and why we need both.

Yeah, so the 'why' is:

 (a) to distinguish the case of "I'm mapping the first sub-page of a
compound page as a _single_ page page entry in the pte" from "I'm
mapping the whole compound/THP/HUGETLB page as a pmd"

The actual 'page' pointer can be the same in both cases, so you can't
tell from that: PageCompound() will be true in both cases.

Of course, sometimes you *can* tell from the page pointer too (eg the
HUGETLB case can never be mapped as a small page), but not always.

 (b) because we do completely different things from a page locking and
statistics standpoint for the two cases.

That (b) is obviously related to (a), but it's effectively the main
reason why rmap needs to be able to tell the difference in the first
place.

> HugeTLB pages _are_ marked as Compound.

Oh, ok. It's not clear why they would be, and historically I don't
think they were, but I guess it's for random implementation details
(probably to look up the head page logic).

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

Yeah, the HUGETLB case is a mess these days, but it made sense
historically, because it was a much simpler thing than the THP pages
that have all the fragmentation cases.

Now that we handle pmd-sized pages anyway, the HUGETLB case is mostly
just a nasty oddity, but we obviously also do the pud case with
HUGETLB.

And who knows what ia64 did with its completely random page-size
thing. I don't even want to think about it, and thankfully these days
I don't feel like I need to care any more ;)

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

Yeah, it would be lovely to make HUGETLB some THP special case some day.

                 Linus
diff mbox series

Patch

diff --git a/mm/rmap.c b/mm/rmap.c
index 512e53cae2ca..4833d28c5e1a 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1311,7 +1311,11 @@  void page_add_anon_rmap(struct page *page,
 	else
 		VM_BUG_ON_PAGE(!PageLocked(page), page);
 
-	if (compound && PageTransHuge(page)) {
+	if (likely(!PageCompound(page))) {
+		first = atomic_inc_and_test(&page->_mapcount);
+		nr = first;
+
+	} else if (compound && PageTransHuge(page)) {
 		lock_compound_mapcounts(page, &mapcounts);
 		first = !mapcounts.compound_mapcount;
 		mapcounts.compound_mapcount++;
@@ -1321,8 +1325,7 @@  void page_add_anon_rmap(struct page *page,
 				nr = nr_subpages_unmapped(page, nr_pmdmapped);
 		}
 		unlock_compound_mapcounts(page, &mapcounts);
-
-	} else if (PageCompound(page)) {
+	} else {
 		struct page *head = compound_head(page);
 
 		lock_compound_mapcounts(head, &mapcounts);
@@ -1330,10 +1333,6 @@  void page_add_anon_rmap(struct page *page,
 		first = subpage_mapcount_inc(page);
 		nr = first && !mapcounts.compound_mapcount;
 		unlock_compound_mapcounts(head, &mapcounts);
-
-	} else {
-		first = atomic_inc_and_test(&page->_mapcount);
-		nr = first;
 	}
 
 	VM_BUG_ON_PAGE(!first && (flags & RMAP_EXCLUSIVE), page);
@@ -1373,20 +1372,23 @@  void page_add_anon_rmap(struct page *page,
 void page_add_new_anon_rmap(struct page *page,
 	struct vm_area_struct *vma, unsigned long address)
 {
-	const bool compound = PageCompound(page);
-	int nr = compound ? thp_nr_pages(page) : 1;
+	int nr;
 
 	VM_BUG_ON_VMA(address < vma->vm_start || address >= vma->vm_end, vma);
 	__SetPageSwapBacked(page);
-	if (compound) {
+
+	if (likely(!PageCompound(page))) {
+		/* increment count (starts at -1) */
+		atomic_set(&page->_mapcount, 0);
+		nr = 1;
+	} else {
 		VM_BUG_ON_PAGE(!PageTransHuge(page), page);
 		/* increment count (starts at -1) */
 		atomic_set(compound_mapcount_ptr(page), 0);
+		nr = thp_nr_pages(page);
 		__mod_lruvec_page_state(page, NR_ANON_THPS, nr);
-	} else {
-		/* increment count (starts at -1) */
-		atomic_set(&page->_mapcount, 0);
 	}
+
 	__mod_lruvec_page_state(page, NR_ANON_MAPPED, nr);
 	__page_set_anon_rmap(page, vma, address, 1);
 }
@@ -1409,7 +1411,11 @@  void page_add_file_rmap(struct page *page,
 	VM_BUG_ON_PAGE(compound && !PageTransHuge(page), page);
 	lock_page_memcg(page);
 
-	if (compound && PageTransHuge(page)) {
+	if (likely(!PageCompound(page))) {
+		first = atomic_inc_and_test(&page->_mapcount);
+		nr = first;
+
+	} else if (compound && PageTransHuge(page)) {
 		lock_compound_mapcounts(page, &mapcounts);
 		first = !mapcounts.compound_mapcount;
 		mapcounts.compound_mapcount++;
@@ -1419,8 +1425,7 @@  void page_add_file_rmap(struct page *page,
 				nr = nr_subpages_unmapped(page, nr_pmdmapped);
 		}
 		unlock_compound_mapcounts(page, &mapcounts);
-
-	} else if (PageCompound(page)) {
+	} else {
 		struct page *head = compound_head(page);
 
 		lock_compound_mapcounts(head, &mapcounts);
@@ -1428,10 +1433,6 @@  void page_add_file_rmap(struct page *page,
 		first = subpage_mapcount_inc(page);
 		nr = first && !mapcounts.compound_mapcount;
 		unlock_compound_mapcounts(head, &mapcounts);
-
-	} else {
-		first = atomic_inc_and_test(&page->_mapcount);
-		nr = first;
 	}
 
 	if (nr_pmdmapped)
@@ -1471,7 +1472,11 @@  void page_remove_rmap(struct page *page,
 	lock_page_memcg(page);
 
 	/* page still mapped by someone else? */
-	if (compound && PageTransHuge(page)) {
+	if (likely(!PageCompound(page))) {
+		last = atomic_add_negative(-1, &page->_mapcount);
+		nr = last;
+
+	} else if (compound && PageTransHuge(page)) {
 		lock_compound_mapcounts(page, &mapcounts);
 		mapcounts.compound_mapcount--;
 		last = !mapcounts.compound_mapcount;
@@ -1481,8 +1486,7 @@  void page_remove_rmap(struct page *page,
 				nr = nr_subpages_unmapped(page, nr_pmdmapped);
 		}
 		unlock_compound_mapcounts(page, &mapcounts);
-
-	} else if (PageCompound(page)) {
+	} else {
 		struct page *head = compound_head(page);
 
 		lock_compound_mapcounts(head, &mapcounts);
@@ -1490,10 +1494,6 @@  void page_remove_rmap(struct page *page,
 		last = subpage_mapcount_dec(page);
 		nr = last && !mapcounts.compound_mapcount;
 		unlock_compound_mapcounts(head, &mapcounts);
-
-	} else {
-		last = atomic_add_negative(-1, &page->_mapcount);
-		nr = last;
 	}
 
 	if (nr_pmdmapped) {