Re: [PATCH v1 06/11] mm: support GUP-triggered unsharing via FAULT_FLAG_UNSHARE (!hugetlb)

[Jason Gunthorpe <jgg@nvidia.com>]

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?

> 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?

Is this common? Linus' prior email was talking as though swap is so
rare we should't optimize for it?
 
> 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?
 
> 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.

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

> 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.
 
> > Then on the WP sides.. Obviously we clear the bit when applying a WP
> > for copy. So all the bad GUP cases are halted now, as with a cleared
> > bit and a != 1 refcount COW must happen.

> But we really want to avoid degrading fork() for everybody that doesn't
> do heavy GUP ...

fork() already has to dirty the struct page cache line for refcount,
setting a flag seems minor at that point? At least we shouldn't
discard this nice understandable approach without a measurement....

Remember fork is already incring mapcount so if we kill mapcount it is
a win for fork to replace the mapcount atomic with a non-atomic flag.

> > AFAIK the only places that can break this are places putting struct
> > page memory into special PTEs. Which is horrific and is just bugs, but
> > I think I've seen it from time to time :(
> 
> As we only care about anon pages, I think that doesn't apply. At least
> that's what I hope.

You are optimistic :)

Jason