On 09.08.23 21:07, Ryan Roberts wrote:
On 09/08/2023 09:32, David Hildenbrand wrote:
Let's track the total mapcount for all large folios in the first subpage.
The total mapcount is what we actually want to know in folio_mapcount()
and it is also sufficient for implementing folio_mapped(). This also
gets rid of any "raceiness" concerns as expressed in
folio_total_mapcount().
With sub-PMD THP becoming more important and things looking promising
that we will soon get support for such anon THP, we want to avoid looping
over all pages of a folio just to calculate the total mapcount. Further,
we may soon want to use the total mapcount in other context more
frequently, so prepare for reading it efficiently and atomically.
Make room for the total mapcount in page[1] of the folio by moving
_nr_pages_mapped to page[2] of the folio: it is not applicable to hugetlb
-- and with the total mapcount in place probably also not desirable even
if PMD-mappable hugetlb pages could get PTE-mapped at some point -- so we
can overlay the hugetlb fields.
Note that we currently don't expect any order-1 compound pages / THP in
rmap code, and that such support is not planned. If ever desired, we could
move the compound mapcount to another page, because it only applies to
PMD-mappable folios that are definitely larger. Let's avoid consuming
more space elsewhere for now -- we might need more space for a different
purpose in some subpages soon.
Maintain the total mapcount also for hugetlb pages. Use the total mapcount
to implement folio_mapcount(), total_mapcount(), folio_mapped() and
page_mapped().
We can now get rid of folio_total_mapcount() and
folio_large_is_mapped(), by just inlining reading of the total mapcount.
_nr_pages_mapped is now only used in rmap code, so not accidentially
externally where it might be used on arbitrary order-1 pages. The remaining
usage is:
(1) Detect how to adjust stats: NR_ANON_MAPPED and NR_FILE_MAPPED
-> If we would account the total folio as mapped when mapping a
page (based on the total mapcount), we could remove that usage.
(2) Detect when to add a folio to the deferred split queue
-> If we would apply a different heuristic, or scan using the rmap on
the memory reclaim path for partially mapped anon folios to
split them, we could remove that usage as well.
So maybe, we can simplify things in the future and remove
_nr_pages_mapped. For now, leave these things as they are, they need more
thought. Hugh really did a nice job implementing that precise tracking
after all.
Note: Not adding order-1 sanity checks to the file_rmap functions for
now. For anon pages, they are certainly not required right now.
Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Cc: Jonathan Corbet <corbet@xxxxxxx>
Cc: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
Cc: Hugh Dickins <hughd@xxxxxxxxxx>
Cc: "Matthew Wilcox (Oracle)" <willy@xxxxxxxxxxxxx>
Cc: Ryan Roberts <ryan.roberts@xxxxxxx>
Cc: Yin Fengwei <fengwei.yin@xxxxxxxxx>
Cc: Yang Shi <shy828301@xxxxxxxxx>
Cc: Zi Yan <ziy@xxxxxxxxxx>
Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
Other than the nits and query on zeroing _total_mapcount below, LGTM. If zeroing
is correct:
Reviewed-by: Ryan Roberts <ryan.roberts@xxxxxxx>
Thanks for the review!
[...]
static inline int total_mapcount(struct page *page)
nit: couldn't total_mapcount() just be implemented as a wrapper around
folio_mapcount()? What's the benefit of PageCompound() check instead of just
getting the folio and checking if it's large? i.e:
Good point, let me take a look tomorrow if the compiler can optimize in
both cases equally well.
[...]
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 5f498e8025cc..6a614c559ccf 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1479,7 +1479,7 @@ static void __destroy_compound_gigantic_folio(struct folio *folio,
struct page *p;
atomic_set(&folio->_entire_mapcount, 0);
- atomic_set(&folio->_nr_pages_mapped, 0);
+ atomic_set(&folio->_total_mapcount, 0);
Just checking this is definitely what you intended? _total_mapcount is -1 when
it means "no pages mapped", so 0 means 1 page mapped?
I was blindly doing what _entire_mapcount is doing: zeroing out the
values. ;)
But let's look into the details: in __destroy_compound_gigantic_folio(),
we're manually dissolving the whole compound page. So instead of
actually returning a compound page to the buddy (where we would make
sure the mapcounts are -1, to then zero them out !), we simply zero-out
the fields we use and then dissolve the compound page: to be left with a
bunch of order-0 pages where the memmap is in a clean state.
(the buddy doesn't handle that page order, so we have to do things
manually to get to order-0 pages we can reuse or free)
--
Cheers,
David / dhildenb
