Date:   Wed, 4 Jan 2023 10:20:51 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Hugh Dickins <hughd@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        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

>> 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.
> 
> 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

Powered by blists - more mailing lists