Date:   Fri, 30 Sep 2022 02:57:40 +0800
From:   Chih-En Lin <shiyn.lin@...il.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     Nadav Amit <namit@...are.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Qi Zheng <zhengqi.arch@...edance.com>,
        Matthew Wilcox <willy@...radead.org>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Iurii Zaikin <yzaikin@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        William Kucharski <william.kucharski@...cle.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Peter Xu <peterx@...hat.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Arnd Bergmann <arnd@...db.de>,
        Tong Tiangen <tongtiangen@...wei.com>,
        Pasha Tatashin <pasha.tatashin@...een.com>,
        Li kunyu <kunyu@...china.com>,
        Anshuman Khandual <anshuman.khandual@....com>,
        Minchan Kim <minchan@...nel.org>,
        Yang Shi <shy828301@...il.com>, Song Liu <song@...nel.org>,
        Miaohe Lin <linmiaohe@...wei.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Andy Lutomirski <luto@...nel.org>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Dinglan Peng <peng301@...due.edu>,
        Pedro Fonseca <pfonseca@...due.edu>,
        Jim Huang <jserv@...s.ncku.edu.tw>,
        Huichun Feng <foxhoundsk.tw@...il.com>
Subject: Re: [RFC PATCH v2 9/9] mm: Introduce Copy-On-Write PTE table

On Thu, Sep 29, 2022 at 08:38:52PM +0200, David Hildenbrand wrote:
> On 29.09.22 20:29, Chih-En Lin wrote:
> > On Thu, Sep 29, 2022 at 07:24:31PM +0200, David Hildenbrand wrote:
> > > > > IMHO, a relaxed form that focuses on only the memory consumption reduction
> > > > > could *possibly* be accepted upstream if it's not too invasive or complex.
> > > > > During fork(), we'd do exactly what we used to do to PTEs (increment
> > > > > mapcount, refcount, trying to clear PageAnonExclusive, map the page R/O,
> > > > > duplicate swap entries; all while holding the page table lock), however,
> > > > > sharing the prepared page table with the child process using COW after we
> > > > > prepared it.
> > > > > 
> > > > > Any (most once we want to *optimize* rmap handling) modification attempts
> > > > > require breaking COW -- copying the page table for the faulting process. But
> > > > > at that point, the PTEs are already write-protected and properly accounted
> > > > > (refcount/mapcount/PageAnonExclusive).
> > > > > 
> > > > > Doing it that way might not require any questionable GUP hacks and swapping,
> > > > > MMU notifiers etc. "might just work as expected" because the accounting
> > > > > remains unchanged" -- we simply de-duplicate the page table itself we'd have
> > > > > after fork and any modification attempts simply replace the mapped copy.
> > > > 
> > > > Agree.
> > > > However for GUP hacks, if we want to do the COW to page table, we still
> > > > need the hacks in this patch (using the COW_PTE_OWN_EXCLUSIVE flag to
> > > > check whether the PTE table is available or not before we do the COW to
> > > > the table). Otherwise, it will be more complicated since it might need
> > > > to handle situations like while preparing the COW work, it just figuring
> > > > out that it needs to duplicate the whole table and roll back (recover
> > > > the state and copy it to new table). Hopefully, I'm not wrong here.
> > > 
> > > The nice thing is that GUP itself *usually* doesn't modify page tables. One
> > > corner case is follow_pfn_pte(). All other modifications should happen in
> > > the actual fault handler that has to deal with such kind of unsharing either
> > > way when modifying the PTE.
> > > 
> > > If the pages are already in a COW-ed pagetable in the desired "shared" state
> > > (e.g., PageAnonExclusive cleared on an anonymous page), R/O pinning of such
> > > pages will just work as expected and we shouldn't be surprised by another
> > > set of GUP+COW CVEs.
> > > 
> > > We'd really only deduplicate the page table and not play other tricks with
> > > the actual page table content that differ from the existing way of handling
> > > fork().
> > > 
> > > I don't immediately see why we need COW_PTE_OWN_EXCLUSIVE in GUP code when
> > > not modifying the page table. I think we only need "we have to unshare this
> > > page table now" in follow_pfn_pte() and inside the fault handling when GUP
> > > triggers a fault.
> > > 
> > > I hope my assumption is correct, or am I missing something?
> > > 
> > 
> > My consideration is when we pinned the page and did the COW to make the
> > page table be shared. It might not allow mapping the pinned page to R/O)
> > into both processes.
> > 
> > So, if the fork is working on the shared state, it needs to recover the
> > table and copy to a new one since that pinned page will need to copy
> > immediately. We can hold the shared state after occurring such a
> > situation. So we still need some trick to let the fork() know which page
> > table already has the pinned page (or such page won't let us share)
> > before going to duplicate.
> > 
> > Am I wrong here?
> 
> I think you might be overthinking this. Let's keep it simple:
> 
> 1) Handle pinned anon pages just as I described below, falling back to the
> "slow" path of page table copying.
> 
> 2) Once we passed that stage, you can be sure that the COW-ed page table
> cannot have actually pinned anon pages. All anon pages in such a page table
> have PageAnonExclusive cleared and are "maybe shared". GUP cannot succeed in
> pinning these pages anymore, because it will only pin exclusive anon pages!
> 
> 3) If anybody wants to take a R/O pin on a shared anon page that is mapped
> into a COW-ed page table, we trigger a fault with FAULT_FLAG_UNSHARE instead
> of pinning the page. This has to break COW on the page table and properly
> map an exclusive anon page into it, breaking COW.
> 
> Do you see a problem with that?
> 
> > 
> > After that, since we handled the accounting in fork(), we don't need
> > ownership (pmd_t pointer) anymore. We have to find another way to mark
> > the table to be exclusive. (Right now, COW_PTE_OWNER_EXCLUSIVE flag is
> > stored at that space.)
> > 
> > > > 
> > > > > But devil is in the detail (page table lock, TLB flushing).
> > > > 
> > > > Sure, it might be an overhead in the page fault and needs to be handled
> > > > carefully. ;)
> > > > 
> > > > > "will make fork() even have more overhead" is not a good excuse for such
> > > > > complexity/hacks -- sure, it will make your benchmark results look better in
> > > > > comparison ;)
> > > > 
> > > > ;);)
> > > > I think that, even if we do the accounting with the COW page table, it
> > > > still has a little bit improve.
> > > 
> > > :)
> > > 
> > > My gut feeling is that this is true. While we have to do a pass over the
> > > parent page table during fork and wrprotect all PTEs etc., we don't have to
> > > duplicate the page table content and allocate/free memory for that.
> > > 
> > > One interesting case is when we cannot share an anon page with the child
> > > process because it maybe pinned -- and we have to copy it via
> > > copy_present_page(). In that case, the page table between the parent and the
> > > child would differ and we'd not be able to share the page table.
> > 
> > That is what I want to say above.
> > The case might happen in the middle of the shared page table progress.
> > It might cost more overhead to recover it. Therefore, if GUP wants to
> > pin the mapped page we can mark the PTE table first, so fork() won't
> > waste time doing the work for sharing.
> 
> Having pinned pages is a corner case for most apps. No need to worry about
> optimizing this corner case for now.
> 
> I see what you are trying to optimize, but I don't think this is needed in a
> first version, and probably never is needed.
> 
> 
> Any attempts to mark page tables in a certain way from GUP
> (COW_PTE_OWNER_EXCLUSIVE) is problematic either way: GUP-fast
> (get_user_pages_fast) can race with pretty much anything, even with
> concurrent fork. I suspect your current code might be really racy in that
> regard.

I see.
Now, I know why optimizing that corner case is not worth it.
Thank you for explaining that.

Thanks,
Chih-En Lin

Powered by blists - more mailing lists