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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b00e2d3a-8601-924c-241c-4373b9dea0cb@redhat.com>
Date:   Wed, 23 Aug 2023 17:17:37 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Yosry Ahmed <yosryahmed@...gle.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        linux-arm-kernel@...ts.infradead.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Matthew Wilcox <willy@...radead.org>,
        Peter Xu <peterx@...hat.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Hugh Dickins <hughd@...gle.com>,
        Seth Jennings <sjenning@...hat.com>,
        Dan Streetman <ddstreet@...e.org>,
        Vitaly Wool <vitaly.wool@...sulko.com>
Subject: Re: [PATCH mm-unstable v1 1/4] mm/swap: stop using page->private on
 tail pages for THP_SWAP

On 23.08.23 17:12, Yosry Ahmed wrote:
> On Mon, Aug 21, 2023 at 9:09 AM David Hildenbrand <david@...hat.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@...hat.com>
> 
> The mm part looks good to me (with the added fixup):
> 
> Reviewed-by: Yosry Ahmed <yosryahmed@...gle.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)

-- 
Cheers,

David / dhildenb