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.
--
Thanks,
David / dhildenb
