Re: [PATCH 1/1] mm/ksm: trylock the mmap lock in the unstable tree walk

[xu.xin16]

> From: Longlong Xia <xialonglong@xxxxxxxxxx>
> 
> Every node that unstable_tree_search_insert() descends through is
> revalidated by get_mergeable_page(), which takes the mmap_read_lock
> of the mm the node's rmap_item belongs to. These are taken while
> ksmd holds ksm_thread_mutex, so a single mm whose mmap lock is
> being written to - a process busily mmap'ing or munmap'ing -
> stalls the whole scanner, for every KSM user on the system.
> 
> Use mmap_read_trylock() instead. The trade-off is that pages of a
> contended mm may need more full scans to merge; in return, ksmd
> latency no longer depends on unrelated mmap activity of the
> scanned processes.
> 
> Testing, on a 4 vCPU QEMU x86_64 guest with ksmd at
> pages_to_scan=100000 and sleep_millisecs=0, 5 runs per kernel
> (median reported; baseline is the parent commit):
> 
> 1. A contender process registers a 64 MiB MADV_MERGEABLE area of
>    unique pages and runs two threads looping mmap/munmap of
>    256 MiB, so its mmap lock is held for writing much of the
>    time.
> 2. Once that churn is running, a quiet victim process registers
>    32 MiB of 2048 unique pages duplicated 4 times.
> 3. Sample /sys/kernel/mm/ksm counters and ksmd's /proc stats
>    every 0.5 s for ~2 min of churn, then stop the churn and
>    sample until the victim finishes merging.  The same phases
>    run without the churn as an uncontended control.
> 
> Results (median of 5 runs):
> 
> - contended ksmd scan rate: 11,473 -> 58,397 pages/s (5.1x)
> - contended victim merge time: 10.4 s -> 1.7 s; the contended
> - contended ksmd CPU per scanned page: 13.4 us -> 3.5 us
> - ksmd time in uninterruptible sleep under churn: 78% -> 57%;
> - uncontended (control): merge time 0.65s vs 0.62s, scan rate
>   259K vs 247K pages/s and ksmd CPU 3.9 vs 4.0 us per page,
>   unchanged within ~5%.

Sorry, I'm not fully convinced by this approach. The performance numbers
indeed show improvements with trylock, but I'm not sure they translate
into real user benefit. Users typically care about how many pages KSM
actually merges and how much memory is saved, not just how fast ksmd scans.

> 
> Assisted-by: Zcode:GLM-5.3
> Signed-off-by: Longlong Xia <xialonglong@xxxxxxxxxx>
> ---
>  mm/ksm.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 49d48d1e0998..3cfb09a926ff 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -820,7 +820,14 @@ static struct page *get_mergeable_page(struct ksm_rmap_item *rmap_item)
>  	struct folio_walk fw;
>  	struct folio *folio;
>  
> -	mmap_read_lock(mm);
> +	/*
> +	 * We trylock because we don't want ksmd to wait for an mm that is
> +	 * busy changing its memory layout: we prefer to skip this page and
> +	 * let the next full scan retry it, like the folio trylock in
> +	 * try_to_merge_one_page().
> +	 */
> +	if (!mmap_read_trylock(mm))
> +		return NULL;
>  	vma = find_mergeable_vma(mm, addr);
>  	if (!vma)
>  		goto out;
> -- 
> 2.43.0
Sorry, NACK

Also, this change feels a bit too blunt to me. In scenarios with even mild
contention (far less than the heavy churn in your test), ksmd could end up repeatedly
failing to acquire the mmap lock and thus fail to merge any pages for a long time.
That could hurt KSM's effectiveness for ordinary workloads, not just the contended ones.

Besides, there are lots of mmap_read_lock in one page's searcing and merging of ksmd, such
as try_to_merge_with_zero_page, try_to_merge_with_ksm_page and so on...

I'd prefer a less aggressive solution that avoids stalling ksmd without starving page
merging entirely. Maybe considering it together with smart scan? Need to see other
suggestions from other maintainers like David.

Thanks,
Xu Xin