Re: [PATCH RFC 07/39] mm/rmap: convert folio_add_file_rmap_range() into folio_add_file_rmap_[pte|ptes|pmd]()

[David Hildenbrand <david@redhat.com>]

On 05.12.23 13:04, Ryan Roberts wrote:
> On 04/12/2023 14:21, David Hildenbrand wrote:
>> Let's get rid of the compound parameter and instead define implicitly
>> which mappings we're adding. That is more future proof, easier to read
>> and harder to mess up.
>>
>> Use an enum to express the granularity internally. Make the compiler
>> always special-case on the granularity by using __always_inline.
>>
>> Add plenty of sanity checks with CONFIG_DEBUG_VM. Replace the
>> folio_test_pmd_mappable() check by a config check in the caller and
>> sanity checks. Convert the single user of folio_add_file_rmap_range().
>>
>> This function design can later easily be extended to PUDs and to batch
>> PMDs. Note that for now we don't support anything bigger than
>> PMD-sized folios (as we cleanly separated hugetlb handling). Sanity checks
> 
> Is that definitely true? Don't we support PUD-mapping file-backed DAX memory?

They are not handled via the rmap. Otherwise, all the PMD accounting 
(e.g., FilePmdMapped) in RMAP code would already be wrong for them.

And it's easy to verify by looking at zap_huge_pud() that doesn't call 
any rmap code.

[...]

>> +
>> +static inline void __folio_rmap_sanity_checks(struct folio *folio,
>> +		struct page *page, unsigned int nr_pages, enum rmap_mode mode)
>> +{
>> +	/* hugetlb folios are handled separately. */
>> +	VM_WARN_ON_FOLIO(folio_test_hugetlb(folio), folio);
>> +	VM_WARN_ON_FOLIO(folio_test_large(folio) &&
>> +			 !folio_test_large_rmappable(folio), folio);
>> +
>> +	VM_WARN_ON_ONCE(!nr_pages || nr_pages > folio_nr_pages(folio));
> 
> nit: I don't think you technically need the second half of this - its covered by
> the test below...

My thinking was that if nr_pages would be "-1", one could end up with 
weird wraparounds.

But yeah, I thought about this as well and might just remove it.

> 
>> +	VM_WARN_ON_FOLIO(page_folio(page) != folio, folio);
>> +	VM_WARN_ON_FOLIO(page_folio(page + nr_pages - 1) != folio, folio);
> 
> ...this one.
> 

Thanks!

-- 
Cheers,

David / dhildenb