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@xxxxxxxxxx>
Cc: Jann Horn <jannh@xxxxxxxxxx>
Cc: Yang Shi <shy828301@xxxxxxxxx>
Cc: David Hildenbrand <david@xxxxxxxxxx>
Cc: Zach O'Keefe <zokeefe@xxxxxxxxxx>
Cc: Song Liu <songliubraving@xxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx> [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.
It's about recovering performance *after* debugging. It is not something
that is of any value to me personally, nor (so far as I know) to anyone
whom I work with. But it is something which Song Liu went to the trouble
to make possible in his "THP aware uprobe" series three years ago, and it
is something which Jann unintentionally regressed in his recent commit:
so I thought it proper to reinstate where regressed.
Right, although I wonder if that original series fixed a real
performance issue or was more a "this makes sense, let's just optimize
this corner case by some serious complexity". I hope it's not the latter :)
(What I do have more of an investment in, is for MADV_COLLAPSE to be able
to collapse some extents in a large vma where some other extent got CoWed,
so giving the whole vma an anon_vma. But that's not an issue for -stable,
and I cannot tell you offhand whether undoing this anon_vma exclusion is
enough to enable that or not - I suspect not, I suspect a result code or
switch statement needs to be adjusted too.)
Yeah, having a single COWed page in a large MAP_PRIVATE file/shmem
mapping would disable collapse, so it's the right thing to do.
Thinking about it some more, and the effective code change, stable
doesn't sound wrong.
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.
I firmly disagree - it's from 2022! and much too small to be ugliest;
but I haven't thought about the aspect that is bothering you there.
The ugliest I stumbled over in early 2023 -- until January 2nd :D
What's bothered me most about it, is the way its name, and the naming of
the do_set_pmd() it interfaces with, give no hint that they are entirely
about file (or shmem) vmas, and would not work right on anon vmas
(I forget whether it's just a matter of which stats updated, or more).
Yes. I dug very deep into in-place collapse yesterday because I was
briefly concerned about anon THP, and it took me longer to understand
that whole machinery than it should (and that anon THP never ever
collapse in-place).
Some of that khugepaged stuff needs some *serious* cleanups and
refactoring. do_set_pmd() is not an exception.
Some more examples:
if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file) {
...
hpage_collapse_scan_file()
} else {
hpage_collapse_scan_pmd()
...
}
1) hpage_collapse_scan_pmd() is only for anon memory. Totally obvious
from the name. But why are we potentially calling it for VMAs that
are not applicable? For maximum David confusion?
2) "IS_ENABLED(CONFIG_SHMEM) && vma->vm_file" is also supposed to cover
ordinary file-thp. Totally obvious from the IS_ENABLED(CONFIG_SHMEM)
... I probably spent 30minutes understanding what's happening here.
Just misleading and wrong without CONFIG_SHMEM.
... and what's easier to get than this magic set of boolean flags:
hugepage_vma_check(vma, vma->vm_flags, false, false, true)
... and obviously
hugepage_vma_revalidate()
is supposed to be a follow up to a previous
hugepage_vma_check()
and totally different from
transhuge_vma_suitable()
Hard to make it even less consistent.
Also, it's very clear from the code that SCAN_PTE_MAPPED_HUGEPAGE only
applies to file-thp, right? No.
--
Thanks,
David / dhildenb
