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

[David Hildenbrand]

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@xxxxxxxxxx>
> Reviewed-by: Yin Fengwei <fengwei.yin@xxxxxxxxx>

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