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

[Fuad Tabba <tabba@google.com>]

On Fri, 17 Jan 2025 at 22:05, Elliot Berman
<elliot.berman@oss.qualcomm.com> 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.
>
> You'll probably want to do the same for the PGTY_guestmem in the later
> patch!

Thanks Elliot. This will keep the kernel test robot happy when I respin.

Cheers,
/fuad

>
> > +     case PGTY_offline:
> > +             /* Nothing to do, it's offline. */
> > +             return;
> > +     default:
> > +             WARN_ON_ONCE(1);
> > +     }
> > +}
> > +
> >  void __folio_put(struct folio *folio)
> >  {
> >       if (unlikely(folio_is_zone_device(folio))) {
> > @@ -101,8 +115,8 @@ void __folio_put(struct folio *folio)
> >               return;
> >       }
> >
> > -     if (folio_test_hugetlb(folio)) {
> > -             free_huge_folio(folio);
> > +     if (unlikely(folio_has_type(folio))) {
> > +             free_typed_folio(folio);
> >               return;
> >       }
> >
> > @@ -934,13 +948,13 @@ void folios_put_refs(struct folio_batch *folios, unsigned int *refs)
> >               if (!folio_ref_sub_and_test(folio, nr_refs))
> >                       continue;
> >
> > -             /* hugetlb has its own memcg */
> > -             if (folio_test_hugetlb(folio)) {
> > +             if (unlikely(folio_has_type(folio))) {
> > +                     /* typed folios have their own memcg, if any */
> >                       if (lruvec) {
> >                               unlock_page_lruvec_irqrestore(lruvec, flags);
> >                               lruvec = NULL;
> >                       }
> > -                     free_huge_folio(folio);
> > +                     free_typed_folio(folio);
> >                       continue;
> >               }
> >               folio_unqueue_deferred_split(folio);
> > --
> > 2.48.0.rc2.279.g1de40edade-goog
> >