linux-kernel - Re: [PATCH RFC v2 6/9] mm/khugepaged: remove reuse_swap

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: <205231d0-2b4e-7d93-1028-2d501c1cbf74@redhat.com>
Date:   Fri, 28 Jan 2022 09:41:16 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Yang Shi <shy828301@...il.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Hugh Dickins <hughd@...gle.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        David Rientjes <rientjes@...gle.com>,
        Shakeel Butt <shakeelb@...gle.com>,
        John Hubbard <jhubbard@...dia.com>,
        Jason Gunthorpe <jgg@...dia.com>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Matthew Wilcox <willy@...radead.org>,
        Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
        Michal Hocko <mhocko@...nel.org>,
        Nadav Amit <namit@...are.com>, Rik van Riel <riel@...riel.com>,
        Roman Gushchin <guro@...com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Peter Xu <peterx@...hat.com>,
        Donald Dutile <ddutile@...hat.com>,
        Christoph Hellwig <hch@....de>,
        Oleg Nesterov <oleg@...hat.com>, Jan Kara <jack@...e.cz>,
        Liang Zhang <zhangliang5@...wei.com>,
        Linux MM <linux-mm@...ck.org>
Subject: Re: [PATCH RFC v2 6/9] mm/khugepaged: remove reuse_swap_page() usage

On 27.01.22 22:23, Yang Shi wrote:
> On Wed, Jan 26, 2022 at 2:00 AM David Hildenbrand <david@...hat.com> wrote:
>>
>> reuse_swap_page() currently indicates if we can write to an anon page
>> without COW. A COW is required if the page is shared by multiple
>> processes (either already mapped or via swap entries) or if there is
>> concurrent writeback that cannot tolerate concurrent page modifications.
>>
>> reuse_swap_page() doesn't check for pending references from other
>> processes that already unmapped the page, however,
>> is_refcount_suitable() essentially does the same thing in the context of
>> khugepaged. khugepaged is the last remaining user of reuse_swap_page() and
>> we want to remove that function.
>>
>> In the context of khugepaged, we are not actually going to write to the
>> page and we don't really care about other processes mapping the page:
>> for example, without swap, we don't care about shared pages at all.
>>
>> The current logic seems to be:
>> * Writable: -> Not shared, but might be in the swapcache. Nobody can
>>   fault it in from the swapcache as there are no other swap entries.
>> * Readable and not in the swapcache: Might be shared (but nobody can
>>   fault it in from the swapcache).
>> * Readable and in the swapcache: Might be shared and someone might be
>>   able to fault it in from the swapcache. Make sure we're the exclusive
>>   owner via reuse_swap_page().
>>
>> Having to guess due to lack of comments and documentation, the current
>> logic really only wants to make sure that a page that might be shared
>> cannot be faulted in from the swapcache while khugepaged is active.
>> It's hard to guess why that is that case and if it's really still required,
>> but let's try keeping that logic unmodified.
> 
> I don't think it could be faulted in while khugepaged is active since
> khugepaged does hold mmap_lock in write mode IIUC. So page fault is
> serialized against khugepaged.

It could get faulted in by another process sharing the page, because we 
only synchronize against the current process.

> 
> My wild guess is that collapsing shared pages was not supported before
> v5.8, so we need reuse_swap_page() to tell us if the page in swap
> cache is shared or not. But it is not true anymore. And khugepaged
> just allocates a THP then copy the data from base pages to huge page
> then replace PTEs to PMD, it doesn't change the content of the page,
> so I failed to see a problem by collapsing a shared page in swap
> cache. But I'm really not entirely sure, I may miss something...


Looking more closely where this logic originates from, it was introduced in:

commit 10359213d05acf804558bda7cc9b8422a828d1cd
Author: Ebru Akagunduz <ebru.akagunduz@...il.com>
Date:   Wed Feb 11 15:28:28 2015 -0800

    mm: incorporate read-only pages into transparent huge pages
    
    This patch aims to improve THP collapse rates, by allowing THP collapse in
    the presence of read-only ptes, like those left in place by do_swap_page
    after a read fault.
    
    Currently THP can collapse 4kB pages into a THP when there are up to
    khugepaged_max_ptes_none pte_none ptes in a 2MB range.  This patch applies
    the same limit for read-only ptes.


The change essentially results in a read-only mapped PTE page getting copied and
mapped writable via a new PMD-mapped THP.

It mentions do_swap_page(), so I assume it just tried to do what do_swap_page()
would do when trying to map a page swapped in from the page cache writable
immediately.

But we differ from do_swap_page() that we're not actually going to map the page
writable, we're going to copy the page (__collapse_huge_page_copy()) and map
the copy writable.

I assume we can remove that logic.

-- 
Thanks,

David / dhildenb