On 12.08.24 20:29, Peter Xu wrote:
On Fri, Aug 09, 2024 at 07:59:58PM +0200, David Hildenbrand wrote:
On 09.08.24 19:15, Peter Xu wrote:
On Fri, Aug 09, 2024 at 06:32:44PM +0200, David Hildenbrand wrote:
On 09.08.24 18:08, Peter Xu wrote:
Teach the fork code to properly copy pfnmaps for pmd/pud levels. Pud is
much easier, the write bit needs to be persisted though for writable and
shared pud mappings like PFNMAP ones, otherwise a follow up write in either
parent or child process will trigger a write fault.
Do the same for pmd level.
Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
---
mm/huge_memory.c | 27 ++++++++++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 6568586b21ab..015c9468eed5 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1375,6 +1375,22 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
pgtable_t pgtable = NULL;
int ret = -ENOMEM;
+ pmd = pmdp_get_lockless(src_pmd);
+ if (unlikely(pmd_special(pmd))) {
+ dst_ptl = pmd_lock(dst_mm, dst_pmd);
+ src_ptl = pmd_lockptr(src_mm, src_pmd);
+ spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
+ /*
+ * No need to recheck the pmd, it can't change with write
+ * mmap lock held here.
+ */
+ if (is_cow_mapping(src_vma->vm_flags) && pmd_write(pmd)) {
+ pmdp_set_wrprotect(src_mm, addr, src_pmd);
+ pmd = pmd_wrprotect(pmd);
+ }
+ goto set_pmd;
+ }
+
I strongly assume we should be using using vm_normal_page_pmd() instead of
pmd_page() further below. pmd_special() should be mostly limited to GUP-fast
and vm_normal_page_pmd().
One thing to mention that it has this:
if (!vma_is_anonymous(dst_vma))
return 0;
Another obscure thing in this function. It's not the job of copy_huge_pmd()
to make the decision whether to copy, it's the job of vma_needs_copy() in
copy_page_range().
And now I have to suspect that uffd-wp is broken with this function, because
as vma_needs_copy() clearly states, we must copy, and we don't do that for
PMDs. Ugh.
What a mess, we should just do what we do for PTEs and we will be fine ;)
IIUC it's not a problem: file uffd-wp is different from anonymous, in that
it pushes everything down to ptes.
It means if we skipped one huge pmd here for file, then it's destined to
have nothing to do with uffd-wp, otherwise it should have already been
split at the first attempt to wr-protect.
Is that also true for UFFD_FEATURE_WP_ASYNC, when we call
pagemap_scan_thp_entry()->make_uffd_wp_pmd() ?
I'm not immediately finding the code that does the "pushes everything
down to ptes", so I might miss that part.
Also, we call copy_huge_pmd() only if "is_swap_pmd(*src_pmd) ||
pmd_trans_huge(*src_pmd) || pmd_devmap(*src_pmd)"
Would that even be the case with PFNMAP? I suspect that pmd_trans_huge()
would return "true" for special pfnmap, which is rather "surprising", but
fortunate for us.
It's definitely not surprising to me as that's the plan.. and I thought it
shoulidn't be surprising to you - if you remember before I sent this one, I
tried to decouple that here with the "thp agnostic" series:
https://lore.kernel.org/r/20240717220219.3743374-1-peterx@xxxxxxxxxx
in which you reviewed it (which I appreciated).
So yes, pfnmap on pmd so far will report pmd_trans_huge==true.
I review way to much stuff to remember everything :) That certainly
screams for a cleanup ...
Likely we should be calling copy_huge_pmd() if pmd_leaf() ... cleanup for
another day.
Yes, ultimately it should really be a pmd_leaf(), but since I didn't get
much feedback there, and that can further postpone this series from being
posted I'm afraid, then I decided to just move on with "taking pfnmap as
THPs". The corresponding change on this path is here in that series:
https://lore.kernel.org/all/20240717220219.3743374-7-peterx@xxxxxxxxxx/
@@ -1235,8 +1235,7 @@ copy_pmd_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
src_pmd = pmd_offset(src_pud, addr);
do {
next = pmd_addr_end(addr, end);
- if (is_swap_pmd(*src_pmd) || pmd_trans_huge(*src_pmd)
- || pmd_devmap(*src_pmd)) {
+ if (is_swap_pmd(*src_pmd) || pmd_is_leaf(*src_pmd)) {
int err;
VM_BUG_ON_VMA(next-addr != HPAGE_PMD_SIZE, src_vma);
err = copy_huge_pmd(dst_mm, src_mm, dst_pmd, src_pmd,
Ah, good.
[...]
Yes, as stated above, likely broken with UFFD-WP ...
I really think we should make this code just behave like it would with PTEs,
instead of throwing in more "different" handling.
So it could simply because file / anon uffd-wp work very differently.
Or because nobody wants to clean up that code ;)
--
Cheers,
David / dhildenb
