lkml.org 
[lkml]   [2023]   [Aug]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH mm-unstable v1 1/4] mm/swap: stop using page->private on tail pages for THP_SWAP
From
On 23.08.23 17:21, Yosry Ahmed wrote:
> On Wed, Aug 23, 2023 at 8:17 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 23.08.23 17:12, Yosry Ahmed wrote:
>>> On Mon, Aug 21, 2023 at 9:09 AM David Hildenbrand <david@redhat.com> 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@redhat.com>
>>>
>>> The mm part looks good to me (with the added fixup):
>>>
>>> Reviewed-by: Yosry Ahmed <yosryahmed@google.com>
>>
>> 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.

--
Cheers,

David / dhildenb

\
 
 \ /
  Last update: 2023-08-23 17:29    [W:0.074 / U:0.596 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site