Date:   Wed, 9 Aug 2023 21:17:31 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Ryan Roberts <ryan.roberts@....com>, linux-kernel@...r.kernel.org
Cc:     linux-mm@...ck.org, linux-doc@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jonathan Corbet <corbet@....net>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Hugh Dickins <hughd@...gle.com>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Yin Fengwei <fengwei.yin@...el.com>,
        Yang Shi <shy828301@...il.com>, Zi Yan <ziy@...dia.com>
Subject: Re: [PATCH mm-unstable v1] mm: add a total mapcount for large folios

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@...ux-foundation.org>
>> Cc: Jonathan Corbet <corbet@....net>
>> Cc: Mike Kravetz <mike.kravetz@...cle.com>
>> Cc: Hugh Dickins <hughd@...gle.com>
>> Cc: "Matthew Wilcox (Oracle)" <willy@...radead.org>
>> Cc: Ryan Roberts <ryan.roberts@....com>
>> Cc: Yin Fengwei <fengwei.yin@...el.com>
>> Cc: Yang Shi <shy828301@...il.com>
>> Cc: Zi Yan <ziy@...dia.com>
>> Signed-off-by: David Hildenbrand <david@...hat.com>
> 
> Other than the nits and query on zeroing _total_mapcount below, LGTM. If zeroing
> is correct:
> 
> Reviewed-by: Ryan Roberts <ryan.roberts@....com>

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

Powered by blists - more mailing lists