Re: [RFC PATCH v5 01/15] mm: Consolidate freeing of typed folios on final folio_put()

[Fuad Tabba <tabba@google.com>]

On Mon, 20 Jan 2025 at 10:39, David Hildenbrand <david@redhat.com> wrote:
>
> On 17.01.25 23:05, Elliot Berman wrote:
> > On Fri, Jan 17, 2025 at 04:29:47PM +0000, Fuad Tabba wrote:
> >> Some folio types, such as hugetlb, handle freeing their own
> >> folios. Moreover, guest_memfd will require being notified once a
> >> folio's reference count reaches 0 to facilitate shared to private
> >> folio conversion, without the folio actually being freed at that
> >> point.
> >>
> >> As a first step towards that, this patch consolidates freeing
> >> folios that have a type. The first user is hugetlb folios. Later
> >> in this patch series, guest_memfd will become the second user of
> >> this.
> >>
> >> Suggested-by: David Hildenbrand <david@redhat.com>
> >> Signed-off-by: Fuad Tabba <tabba@google.com>
> >> ---
> >>   include/linux/page-flags.h | 15 +++++++++++++++
> >>   mm/swap.c                  | 24 +++++++++++++++++++-----
> >>   2 files changed, 34 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> >> index 691506bdf2c5..6615f2f59144 100644
> >> --- a/include/linux/page-flags.h
> >> +++ b/include/linux/page-flags.h
> >> @@ -962,6 +962,21 @@ static inline bool page_has_type(const struct page *page)
> >>      return page_mapcount_is_type(data_race(page->page_type));
> >>   }
> >>
> >> +static inline int page_get_type(const struct page *page)
> >> +{
> >> +    return page->page_type >> 24;
> >> +}
> >> +
> >> +static inline bool folio_has_type(const struct folio *folio)
> >> +{
> >> +    return page_has_type(&folio->page);
> >> +}
> >> +
> >> +static inline int folio_get_type(const struct folio *folio)
> >> +{
> >> +    return page_get_type(&folio->page);
> >> +}
> >> +
> >>   #define FOLIO_TYPE_OPS(lname, fname)                                       \
> >>   static __always_inline bool folio_test_##fname(const struct folio *folio) \
> >>   {                                                                  \
> >> diff --git a/mm/swap.c b/mm/swap.c
> >> index 10decd9dffa1..6f01b56bce13 100644
> >> --- a/mm/swap.c
> >> +++ b/mm/swap.c
> >> @@ -94,6 +94,20 @@ static void page_cache_release(struct folio *folio)
> >>              unlock_page_lruvec_irqrestore(lruvec, flags);
> >>   }
> >>
> >> +static void free_typed_folio(struct folio *folio)
> >> +{
> >> +    switch (folio_get_type(folio)) {
> >
> > I think you need:
> >
> > +#if IS_ENABLED(CONFIG_HUGETLBFS)
> >> +    case PGTY_hugetlb:
> >> +            free_huge_folio(folio);
> >> +            return;
> > +#endif
> >
> > I think this worked before because folio_test_hugetlb was defined by:
> > FOLIO_TEST_FLAG_FALSE(hugetlb)
> > and evidently compiler optimizes out the free_huge_folio(folio) before
> > linking.
>
> Likely, we should be using
>
>         case PGTY_hugetlb:
>                 if(IF_ENABLED(CONFIG_HUGETLBFS))
>                         free_huge_folio(folio);
>                 return:
>
> if possible (I assume so).

Yes it does. I'll fix it.

Cheers,
/fuad

> --
> Cheers,
>
> David / dhildenb
>