Re: [PATCH] mm/khugepaged: fix collapse_pte_mapped_thp() to allow anon_vma

[David Hildenbrand <david@redhat.com>]

On 05.01.23 01:03, Yang Shi wrote:
> On Wed, Jan 4, 2023 at 1:20 AM David Hildenbrand <david@redhat.com> wrote:
>>
>>>> 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.
>>>
>>> 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?
> 
> IIRC the VMAs are checked before, what do you mean about "not
> applicable"? But anyway khugepaged/MADV_COLLAPSE does release and

I assume when CONFIG_SHMEM=n with ordinary file-thp we'll end up calling it.

> reacquire mmap_lock multiple times, so there are multiple places to
> check VMAs validity.
> 

hpage_collapse_scan_pmd() should be renamed to something like 
hpage_collapse_scan_an/on() and the duplicate code in 
khugepaged_scan_mm_slot() and madvise_collapse() should be factored out 
into something like:

hpage_collapse_scan(vma, addr, cc)
{
	if (vma->vm_file) {
		...
		hpage_collapse_scan_file()
		...
	} else if (vma_is_anonymous(vma)) {
		hpage_collapse_scan_anon()
	} else {
		WARN_ON_ONCE();
	}
}

Any CONFIG_SHMEM etc. optimizations to compile that code out should go 
into hpage_collapse_scan_file() IMHO. ... also properly checking for 
ordinary file THP support.

... and we'd really decide on a terminology "transhuge", "hugepage", 
"hpage", it's a mess. hpage might be easiest, or simply "thp". We just 
need a way to distinguish all that stuff from hugetlb.

>>
>> 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)
> 
> This is not perfect. I was thinking about changing them to one flag,
> just like TTU_ flags used by try_to_unmap(). That may make things
> cleaner.
> 

We should provide similar flags to hugepage_vma_revalidate() and just 
replace the "cc" parameter by a way to indicate is_khugepaged. Passing 
in cc is just overkill.

We'd name the functions thp_vma_validate() and thp_vma_revalidate() or 
sth. like that.

>>
>> ... 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.
> 
> This was after my cleanup, it was much messier before. And I did add
> comments to make them more understandable, but anyway better naming is
> definitely welcome.

Yeah, I appreciate any previous and any future cleanups in that area.

For example: why even *care* about the complexity of installing a PMD in 
collapse_pte_mapped_thp() using set_huge_pmd() just for MADV_COLLAPSE?

Sure, we avoid a single page fault afterwards, but is this *really* 
worth the extra code here? I mean, after we installed the PMD, the page 
could just get reclaimed either way, so there is no guarantee that we 
have a PMD mapped once we return to user space IIUC.


Anyhow, don't want to hijack this thread. I was just forced to 
understand  that code and a lot jumped at me :)

-- 
Thanks,

David / dhildenb