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

[David Hildenbrand <david@redhat.com>]

On 13.12.23 06:33, Yin Fengwei wrote:
> 
> 
> On 2023/12/11 23:56, 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. Replace
>> the "compound" check by a switch-case that will be removed by the
>> compiler completely.
>>
>> 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
>> will catch if that ever changes.
>>
>> Next up is removing page_remove_rmap() along with its "compound"
>> parameter and smilarly converting all other rmap functions.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Yin Fengwei <fengwei.yin@intel.com>
> 

Thanks!

> With one small comment.
> 
>> ---
>>    include/linux/rmap.h | 47 +++++++++++++++++++++++++--
>>    mm/memory.c          |  2 +-
>>    mm/rmap.c            | 75 +++++++++++++++++++++++++++++---------------
>>    3 files changed, 95 insertions(+), 29 deletions(-)
>>
>> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
>> index e3857d26b944..1753900f4aed 100644
>> --- a/include/linux/rmap.h
>> +++ b/include/linux/rmap.h
>> @@ -191,6 +191,45 @@ typedef int __bitwise rmap_t;
>>     */
>>    #define RMAP_COMPOUND		((__force rmap_t)BIT(1))
>>    
>> +/*
>> + * Internally, we're using an enum to specify the granularity. Usually,
>> + * we make the compiler create specialized variants for the different
>> + * granularity.
>> + */
>> +enum rmap_mode {
>> +	RMAP_MODE_PTE = 0,
>> +	RMAP_MODE_PMD,
>> +};
> Maybe rmap_level for enum name? To me, PTE and PMD are level instead of
> mode.

Originally, I wanted to call this "enum rmap_granularity", but that 
turned out rather long. Agreed that "level" is better than "mode", 
something resembling "granularity" would be even better.

-- 
Cheers,

David / dhildenb