lkml.org 
[lkml]   [2023]   [Jan]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2023-03-26 23:24    [W:0.060 / U:0.496 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site