Messages in this thread | | | Date | Mon, 2 Jan 2023 13:19:26 +0100 | From | David Hildenbrand <> | Subject | Re: [PATCH] mm/khugepaged: fix collapse_pte_mapped_thp() to allow anon_vma |
| |
On 22.12.22 21:41, Hugh Dickins wrote: > uprobe_write_opcode() uses collapse_pte_mapped_thp() to restore huge pmd, > when removing a breakpoint from hugepage text: vma->anon_vma is always > set in that case, so undo the prohibition. And MADV_COLLAPSE ought to be > able to collapse some page tables in a vma which happens to have anon_vma > set from CoWing elsewhere. >
Just so I get this correctly: the degradation is that we won't re-collapse a THP after removing a breakpoint. Certainly "sub-optimal", but I guess nothing that particularly matters in practice.
Or am I wrong?
> Is anon_vma lock required? Almost not: if any page other than expected > subpage of the non-anon huge page is found in the page table, collapse is > aborted without making any change. However, it is possible that an anon > page was CoWed from this extent in another mm or vma, in which case a > concurrent lookup might look here: so keep it away while clearing pmd > (but perhaps we shall go back to using pmd_lock() there in future). > > Note that collapse_pte_mapped_thp() is exceptional in freeing a page table > without having cleared its ptes: I'm uneasy about that, and had thought > pte_clear()ing appropriate; but exclusive i_mmap lock does fix the problem, > and we would have to move the mmu_notification if clearing those ptes. > > Fixes: 8d3c106e19e8 ("mm/khugepaged: take the right locks for page table retraction") > Signed-off-by: Hugh Dickins <hughd@google.com> > Cc: Jann Horn <jannh@google.com> > Cc: Yang Shi <shy828301@gmail.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Zach O'Keefe <zokeefe@google.com> > Cc: Song Liu <songliubraving@fb.com> > Cc: <stable@vger.kernel.org> [5.4+] > --- > What this fixes is not a dangerous instability! But I suggest Cc stable > because uprobes "healing" has regressed in that way, so this should follow > 8d3c106e19e8 into those stable releases where it was backported (and may > want adjustment there - I'll supply backports as needed).
If it's really something that doesn't matter in practice (e.g., -1% performance while debugging :) ), I guess no CC is needed. If there are real production workloads that suffer, I guess ccing stable is fine.
> > mm/khugepaged.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > --- 6.2-rc/mm/khugepaged.c > +++ linux/mm/khugepaged.c > @@ -1460,14 +1460,6 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr, > if (!hugepage_vma_check(vma, vma->vm_flags, false, false, false)) > return SCAN_VMA_CHECK; > > - /* > - * Symmetry with retract_page_tables(): Exclude MAP_PRIVATE mappings > - * that got written to. Without this, we'd have to also lock the > - * anon_vma if one exists. > - */ > - if (vma->anon_vma) > - return SCAN_VMA_CHECK; > - > /* Keep pmd pgtable for uffd-wp; see comment in retract_page_tables() */ > if (userfaultfd_wp(vma)) > return SCAN_PTE_UFFD_WP; > @@ -1567,8 +1559,14 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr, > } > > /* step 4: remove pte entries */ > + /* we make no change to anon, but protect concurrent anon page lookup */ > + if (vma->anon_vma) > + anon_vma_lock_write(vma->anon_vma); > + > collapse_and_free_pmd(mm, vma, haddr, pmd); > > + if (vma->anon_vma) > + anon_vma_unlock_write(vma->anon_vma); > i_mmap_unlock_write(vma->vm_file->f_mapping); > > maybe_install_pmd: >
That code is 99% black magic to me, but staring at the original fix and at collapse_and_free_pmd() makes me assume that grabbing that lock most certainly won't hurt and should be the right thing to do
Acked-by: David Hildenbrand <david@redhat.com>
Side note: set_huge_pmd() wins the award of "ugliest mm function of early 2023". I was briefly concerned how do_set_pmd() decides whether the PMD can be writable or not. Turns out it's communicated via vm_fault->flags. Just horrible.
-- Thanks,
David / dhildenb
| |