Re: [PATCH mm-unstable v1 1/4] mm/swap: stop using page->private on tail pages for THP_SWAP

[David Hildenbrand]

On 23.08.23 17:12, Yosry Ahmed wrote:
> On Mon, Aug 21, 2023 at 9:09 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
> > Let's stop using page->private on tail pages, making it possible to
> > just unconditionally reuse that field in the tail pages of large folios.
> >
> > The remaining usage of the private field for THP_SWAP is in the THP
> > splitting code (mm/huge_memory.c), that we'll handle separately later.
> >
> > Update the THP_SWAP documentation and sanity checks in mm_types.h and
> > __split_huge_page_tail().
> >
> > Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>
> The mm part looks good to me (with the added fixup):
>
> Reviewed-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx>

Thanks!

> >   /**
> > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > index bb5adc604144..84fe0e94f5cd 100644
> > --- a/include/linux/swap.h
> > +++ b/include/linux/swap.h
> > @@ -339,6 +339,15 @@ static inline swp_entry_t folio_swap_entry(struct folio *folio)
> >          return entry;
> >   }
> >
> > +static inline swp_entry_t page_swap_entry(struct page *page)
> > +{
> > +       struct folio *folio = page_folio(page);
> > +       swp_entry_t entry = folio_swap_entry(folio);
> > +
> > +       entry.val += page - &folio->page;
> > +       return entry;
> > +}
> > +
> >   static inline void folio_set_swap_entry(struct folio *folio, swp_entry_t entry)
> >   {
> >          folio->private = (void *)entry.val;
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index cc2f65f8cc62..c04702ae71d2 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2446,18 +2446,15 @@ static void __split_huge_page_tail(struct page *head, int tail,
> >          page_tail->index = head->index + tail;
> >
> >          /*
> > -        * page->private should not be set in tail pages with the exception
> > -        * of swap cache pages that store the swp_entry_t in tail pages.
> > -        * Fix up and warn once if private is unexpectedly set.
> > -        *
> > -        * What of 32-bit systems, on which folio->_pincount overlays
> > -        * head[1].private?  No problem: THP_SWAP is not enabled on 32-bit, and
> > -        * pincount must be 0 for folio_ref_freeze() to have succeeded.
> > +        * page->private should not be set in tail pages. Fix up and warn once
> > +        * if private is unexpectedly set.
> >           */
> > -       if (!folio_test_swapcache(page_folio(head))) {
> > -               VM_WARN_ON_ONCE_PAGE(page_tail->private != 0, page_tail);
> > +       if (unlikely(page_tail->private)) {
> > +               VM_WARN_ON_ONCE_PAGE(true, page_tail);
> >                  page_tail->private = 0;
> >          }
>
> Could probably save a couple of lines here:
>
> if (VM_WARN_ON_ONCE_PAGE(page_tail->private != 0, page_tail))
>
>         page_tail->private = 0;

That would mean that we eventually compile out the runtime check

#define VM_WARN_ON_ONCE_PAGE(cond, page)  BUILD_BUG_ON_INVALID(cond)

--
Cheers,

David / dhildenb