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

David Hildenbrand
2023-12-13T08:47:43Z

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