Message-ID: <303f21d3-42b4-2f11-3f22-28f89f819080@redhat.com>
Date:   Tue, 21 Dec 2021 16:19:33 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Nadav Amit <namit@...are.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Hugh Dickins <hughd@...gle.com>,
        David Rientjes <rientjes@...gle.com>,
        Shakeel Butt <shakeelb@...gle.com>,
        John Hubbard <jhubbard@...dia.com>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Yang Shi <shy828301@...il.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>,
        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>,
        Linux-MM <linux-mm@...ck.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>
Subject: Re: [PATCH v1 06/11] mm: support GUP-triggered unsharing via
 FAULT_FLAG_UNSHARE (!hugetlb)

On 21.12.21 15:28, Jason Gunthorpe wrote:
> On Tue, Dec 21, 2021 at 09:58:32AM +0100, David Hildenbrand wrote:
>>> I'm having a hard time imagining how gup_fast can maintain any sort of
>>> bit - it lacks all forms of locks so how can we do an atomic test and
>>> set between two pieces of data?
>>
>> And exactly that is to be figured out.
>>
>> Note that I am trying to make also any kind of R/O pins on an anonymous
>> page work as expected as well, to fix any kind of GUP after fork() and
>> GUP before fork(). So taking a R/O pin on an !PageAnonExclusive() page
>> similarly has to make sure that the page is exclusive -- even if it's
>> mapped R/O (!).
> 
> Why? AFAIK we don't have bugs here. If the page is RO and has an
> elevated refcount it cannot be 'PageAnonExclusive' and so any place
> that wants to drop the WP just cannot. What is the issue?

Sure it can.

1. Map page R/W
2. Pin it R/W
3. Swapout
4. Read access

Page is now mapped R/O and *has to be* marked PageAnonExclusive(), to
properly skip the COW fault. That's literally 60% of the reproducers we
have that need fixing.


But what I think you actually mean is if we want to get R/O pins right.
> 
>> BUT, it would mean that whenever we fork() and there is one additional
>> reference on a page (even if it's from the swapcache), we would slow
>> down fork() even if there was never any GUP. This would apply to any
>> process out there that does a fork() ...
> 
> You mean because we'd copy?

Yes.

> 
> Is this common? Linus' prior email was talking as though swap is so
> rare we should't optimize for it?
At least in the enterprise segment having swap enabled is mostly a hard
documented requirement. On customer installations swap is still common,
and even gets replaced zswap that is enabled automatically in many
installations ...

So in the world I live and work in, swap is used frequently.

>  
>> So the idea is to mark a page only exclusive as soon as someone needs
>> the page to be exclusive and stay exclusive (-> e.g., GUP with FOLL_PIN
>> or selected FOLL_GET like O_DIRECT). This can happen in my current
>> approach using two ways:
>>
>> (1) Set the bit when we know we are the only users
>>
>> We can set PageAnonExclusive() in case *we sync against fork* and the
>> page cannot get unmapped (pt lock) when:
>> * The page is mapped writable
>> * The page is mapped readable and page_count == 1
> 
> I'm still not sure I see that all this complexity is netting a gain?

Avoid copy on fork().

>  
>> If we cannot set the page exclusive, we have to trigger a page fault.
>>
>> (2) During pagefaults when FOLL_FAULT_UNSHARE is set.
> 
> Why do we need FOLL_FAULT_UNSHARE ? AFAICT that was part of this
> series because of mapcount, once the hugetlb COW is fixed to use
> refcount properly, as Linus showed, the bugs this was trying to fix go
> away.

The purpose of FOLL_FAULT_UNSHARE in the !mapcount version is to cleanly
support R/O pins without the need for FOLL_WRITE.

And it's comparatively easy to add on top. This is not core of the
complexity, really.

> 
> And as discussed before it is OK if READ gup becomes incoherent, that
> is its defined semantic.

And that's where I still disagree.

But anyhow, this is really more about FOLL_FAULT_UNSHARE, which is
pretty easy and natural to add on top and just gets this right.

> 
>> The above should work fairly reliable with GUP. But indeed,
>> gup-fast-only is the problem. I'm still investigating what kind of
>> lightweight synchronization we could do against fork() such that we
>> wouldn't try setting a page PageAnonExclusive() while fork()
>> concurrently shares the page.
>>
>> We could eventually use the page lock and do a try_lock(), both in
>> fork() and in gup-fast-only. fork() would only clear the bit if the
>> try_lock() succeeded. gup-fast-only would only be able to set the bit
>> and not fallback to the slow path if try_lock() succeeded.
> 
> I suspect that is worse than just having fork clear the bit and leave
> GUP as-is. try lock is an atomic, clearing PageAnonExclusive does not
> need to be atomic, it is protected by the PTL.

There are 2 models, leaving FOLL_FAULT_UNSHARE out of the picture for now:

1) Whenever mapping an anonymous page R/W (after COW, during ordinary
fault, on swapin), we mark the page exclusive. We must never lose the
PageAnonExclusive bit, not during migration, not during swapout.

fork() will process the bit for each and every process, even if there
was no GUP, and will copy if there are additional references.

2) Whenever GUP wants to pin/ref a page, we try marking it exclusive. We
can lose the PageAnonExclusive bit during migration and swapout, because
that can only happen when there are no additional references.

fork() will process the bit only if there was GUP. Ordinary fork() is
left unchanged.


Getting R/O supported in the same way just means that we have to check
on a R/O pin if the page is PageAnonExclusive, and if that's not the
case, trigger a FOLL_FAULT_UNSHARE fault. That's really the only
"complexity" on top which is without the mapcount really easy.

-- 
Thanks,

David / dhildenb

Powered by blists - more mailing lists