On Wed, Aug 23, 2023 at 8:26 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> On 23.08.23 17:21, Yosry Ahmed wrote:
> > On Wed, Aug 23, 2023 at 8:17 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
> >>
> >> 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)
> >
> > I thought the warning would be compiled out but not the check, my bad.
>
> I even remembered that VM_WARN_ON_ONCE and friends could/should not be
> used in conditionals.
>
> But we do seem to have two users now:
>
> $ git grep "if (VM_WARN_ON"
> mm/mmap.c: if (VM_WARN_ON_ONCE_MM(vma->vm_end != vmi_end, mm))
> mm/mmap.c: if (VM_WARN_ON_ONCE_MM(vma->vm_start != vmi_start, mm))
>
> But they only do warning-related action, to dump the stack, the vma, ...
>
> So if the warnings get compiled out, also all the other stuff gets compiled out as well,
> which makes sense here.
Funny enough, I did the same grep and immediately thought that since
we have users of that, then it's okay (i.e the check wouldn't be
compiled out). I wasn't thorough enough to actually check what they
are doing :)
>
> --
> Cheers,
>
> David / dhildenb
>
