Re: [PATCH 2/5] mm/khugepaged: scan mm's more efficiently under mmap_lock contention

[David Hildenbrand (Arm)]

On 11/3/26 09:27, Lorenzo Stoakes (ARM) wrote:
> Ostensibly khugepaged collapse traverses eligible mm's, scanning up to

"Ostensibly", now I need a dictionary to read commit messages :P

> /sys/kernel/mm/transparent_hugepage/khugepaged/pages_to_scan eligible pages
> at a time, sleeping for /sys/.../scan_sleep_millisecs between each
> traversal.
> 
> mm's are scanned in collapse_scan_mm_slot() under the mmap read lock,
> obtained using mmap_read_trylock(), skipping the mm if there is lock
> contention, and dropping the lock before allocation.
> 
> khugepaged will keep scanning the same mm until it is about to be torn
> down, prctl(PR_SET_THP_DISABLE) disables THP for it, mm->mmap_lock
> contention occurs, or all its VMAs have been scanned.
> 
> When any of these occur, khugepaged doesn't keep track of where it left
> off, as the current address is tracked in
> khugepaged_scan.address (essentially a global value), which is reset.
> 
> This is a problem when lock contention occurs, as when the mm is next
> scanned, it starts from address 0 again.
> 
> If there's a lot of mmap_lock contention this can significantly slow down
> collapse later in the mm, since the /sys/.../pages_to_scan budget can run
> out before the end of the mm is reached.
> 
> The budget is incremented (as tracked in cc->progress) at a rate of 1 page
> per ineligible VMA, but the budget is only checked once eligible VMAs are
> encountered.
> 
> Then, it only takes the combined total of ineligible VMAs and eligible but
> non-collapsible pages to exceed the /sys/.../pages_to_scan budget for the
> rest of the mm to be skipped.
> 
> For a system with 4 KiB page size and default sysctl settings this only
> requires 4096 ineligible VMAs/uncollapsible pages to be skipped before this
> happens.
> 
> Examples of eligible but uncollapsible ranges are those containing a large
> number of forked PTE entries (more than /sys/.../max_ptes_shared), swapped
> out PTE entries (more than /sys/.../max_ptes_swap) or none or zero-page PTE
> entries (more than /sys/.../max_ptes_none).
> 
> The latter might seem unlikely to happen given /sys/.../max_ptes_none
> defaults to HPAGE_PMD_NR - 1 and CONFIG_PT_RECLAIM frees PTE page tables,
> however CONFIG_PT_RECLAIM only removes the page table if the zapped range
> spans at least a PMD size (see pte_table_reclaim_possible()).
> 
> Thus, userland software that applies madvise(..., MADV_DONTNEED) across
> sub-PMD ranges can thwart this, which is quite feasible for memory
> allocators and GC heaps, for instance.
> 
> Resolve the issue by simply tracking the last examined address in the
> khugepaged_mm_slot object, so when the mm is resumed, the scan starts from
> where it left off.
> 
> Additionally, skip to the next candidate if a VMA cannot be collapsed.
> 
> This issue was noticed during some work in which an mm was instantiated
> with a large number of small VMAs under significant mmap_lock contention,
> during which collapse was observed to take 10.8s. With this patch applied
> it took 3.6s to collapse the same mm.
> 
> Also, while here, update a couple VM_BUG_ON()'s to VM_WARN_ON_ONCE()'s.
> 
> This patch is also a pre-requisite for a later patch in this series.
> 
> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> ---
>  mm/khugepaged.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index aad8797f8d5a..216c4b8d28c4 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -69,23 +69,23 @@ static struct kmem_cache *mm_slot_cache __ro_after_init;
>  /**
>   * struct khugepaged_mm_slot - khugepaged information per mm that is being scanned
>   * @slot: hash lookup from mm to mm_slot
> + * @address: the next address inside that to be scanned
>   */
>  struct khugepaged_mm_slot {
>  	struct mm_slot slot;
> +	unsigned long address;
>  };

I could have sworn I saw a similar patch from Nico at some point ... maybe it
was in a private discussion.


Am I correct or is the short version of this change "Remember the next scan
address per MM instead of starting from address 0 whenever we continue scanning
a MM after a skip; and we might skip under mmap lock contention" ?

I think a lot of the commit message above can be simplified.

Reviewed-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

-- 
Cheers,

David