[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <dc9f7bb0-77d4-8b0e-c6b6-ece3f77ed326@redhat.com>
Date: Mon, 2 Jan 2023 13:19:26 +0100
From: David Hildenbrand <david@...hat.com>
To: Hugh Dickins <hughd@...gle.com>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: Jann Horn <jannh@...gle.com>, Yang Shi <shy828301@...il.com>,
Zach O'Keefe <zokeefe@...gle.com>,
Song Liu <songliubraving@...com>, linux-kernel@...r.kernel.org,
linux-mm@...ck.org
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@...gle.com>
> Cc: Jann Horn <jannh@...gle.com>
> Cc: Yang Shi <shy828301@...il.com>
> Cc: David Hildenbrand <david@...hat.com>
> Cc: Zach O'Keefe <zokeefe@...gle.com>
> Cc: Song Liu <songliubraving@...com>
> Cc: <stable@...r.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@...hat.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
Powered by blists - more mailing lists