Re: [PATCH 2/4] ksm: skip rmap_item allocation for non-zero pages under only_merge_zero_pages

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Tue, Nov 10, 2026 at 03:44:49PM +0800, Xu Xin (vivo) wrote:
> When only_merge_zero_pages is enabled, ksmd only ever merges empty
> (zeroed) anonymous pages with the kernel zero page. A page that carries
> data can never be merged, yet the current code still allocates an
> rmap_item for it in scan_get_next_rmap_item() before cmp_and_merge_page()
> decides to skip it.
>
> On workloads with a large amount of non-mergeable anonymous memory this
> is a real cost: every scanned non-zero page pays for an rmap_item
> allocation, and the rmap_items accumulate in the per-mm pending list.
> For example, a MySQL instance with hundreds of gigabytes of anonymous
> memory (mostly buffer pool pages that cannot be merged) ends up with
> ksmd creating rmap_item metadata for each candidate page; the metadata
> overhead can wipe out any gain from the zero-page merging and even turn
> the net benefit negative.
>
> Add should_skip_non_zero_page(), called in scan_get_next_rmap_item() before
> get_next_rmap_item(), which, when only_merge_zero_pages is enabled,
> checks whether the candidate page is all zeros. A non-zero page is
> skipped entirely: no rmap_item is allocated for it, and any stale
> pending rmap_items below it are freed (as get_next_rmap_item() would).
> The page is reported back to ksm_do_scan() as ERR_PTR(-EAGAIN), which
> counts it as scanned and continues, so the pages_to_scan budget is
> preserved.
>
> The all-zero check (pages_identical() against the kernel zero page) reads
> the whole page. For a non-zero page this is a net win: the extra page read
> is cheaper than the rmap_item allocation and the stable/unstable tree
> operations it would otherwise trigger.
>
> A page that is zero now but is written before cmp_and_merge_page() runs
> is still handled correctly, because the checksum-based decision in
> cmp_and_merge_page() (added by the previous patch) is the authoritative
> one.
>

Please re-write this in your own words.

It's rambling on and never gets to the point of _what this patch does and
why_.

I'm not interested in reading a novel to understand the code :)

It seems very LLM generated, LLMs are _terrible_ at writing commit messages
and commits, please re-write all of that.

> Signed-off-by: Xu Xin (vivo) <xu.xin@linux.dev>
> Co-developed-by: Jinjiang Tu <tujinjiang@huawei.com>
> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>

Why are this and the other patches in-reply-to 1/4? That's not how you send
a series in mm.

> ---
>  mm/ksm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
>
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 49f3728a3d03..e641c37ee096 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -2535,6 +2535,69 @@ static bool should_skip_rmap_item(struct folio *folio,
>  	return true;
>  }
>
> +/*
> + * Determines whether a candidate page should be skipped entirely when
> + * only_merge_zero_pages is enabled, before an rmap_item is allocated for
> + * it.
> + *
> + * A page that is not all zeros carries data and can never be merged with
> + * the kernel zero page, so there is no point in allocating an rmap_item
> + * for it. Skipping it here avoids the rmap_item metadata cost that full
> + * KSM would otherwise pay for every non-mergeable page.
> + *
> + * @rmap_list: head of the per-mm pending rmap_item list
> + * @addr:      address of the candidate page
> + * @folio:     folio containing the candidate page
> + * @page:      the candidate page
> + *
> + * Returns true if the page should be skipped (and any stale pending
> + * rmap_items below @addr are freed), false if it should be processed.
> + */

This is a really weird mix of kdoc style + not kdoc style while not being a
kdoc, for a static function?

> +static bool should_skip_non_zero_page(struct ksm_rmap_item **rmap_list,
> +			     unsigned long addr, struct folio *folio,
> +			     struct page *page)
> +{
> +	struct ksm_rmap_item *rmap_item;
> +
> +	if (!ksm_only_merge_zero_pages)
> +		return false;
> +
> +	/*
> +	 * Pages already merged by KSM are handled by cmp_and_merge_page();
> +	 * do not skip them here.
> +	 */
> +	if (folio_test_ksm(folio))
> +		return false;
> +
> +	/*
> +	 * An all-zero page is exactly what we want to merge with the kernel
> +	 * zero page; process it.
> +	 */
> +	if (pages_identical(page, ZERO_PAGE(0)))
> +		return false;
> +
> +	/*
> +	 * The page carries data: free any pending rmap_items that are below
> +	 * it (they are stale, as get_next_rmap_item() would), and skip it so
> +	 * no rmap_item is allocated for it.
> +	 *
> +	 * Compare the masked address, unlike get_next_rmap_item() which
> +	 * compares the raw rmap_item->address (which may carry the
> +	 * UNSTABLE/STABLE_FLAG bits). That is intentional: a pending
> +	 * rmap_item at the same address as a non-zero page is stale and must
> +	 * be freed here, not returned for processing.
> +	 */

We don't need a prose version of the code, and it reads like AI slop, we
don't want AI slop comments, thanks.

> +	while (*rmap_list) {
> +		rmap_item = *rmap_list;

Why is rmap_item function-scoped when it could be block-scoped here...

> +		if ((rmap_item->address & PAGE_MASK) > addr)
> +			break;
> +		*rmap_list = rmap_item->rmap_list;
> +		remove_rmap_item_from_tree(rmap_item);
> +		free_rmap_item(rmap_item);
> +	}

This code is very ugly.

Newline...

> +	return true;


> +}
> +
>  struct ksm_next_page_arg {
>  	struct folio *folio;
>  	struct page *page;
> @@ -2737,6 +2800,16 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>  			if (tmp_page) {
>  				flush_anon_page(vma, tmp_page, ksm_scan.address);
>  				flush_dcache_page(tmp_page);
> +
> +				if (should_skip_non_zero_page(ksm_scan.rmap_list,
> +						     ksm_scan.address, folio,
> +						     tmp_page)) {
> +					folio_put(folio);
> +					ksm_scan.address += PAGE_SIZE;
> +					mmap_read_unlock(mm);
> +					return ERR_PTR(-EAGAIN);
> +				}
> +

I don't love just shoving more logic into an already massive function. No
to this.

At some point technical debt has to be paid, clean up this code first please.

>  				rmap_item = get_next_rmap_item(mm_slot,
>  					ksm_scan.rmap_list, ksm_scan.address);
>  				if (rmap_item) {
> @@ -2839,6 +2912,15 @@ static void ksm_do_scan(unsigned int scan_npages)
>  		rmap_item = scan_get_next_rmap_item(&page);
>  		if (!rmap_item)
>  			return;
> +		if (rmap_item == ERR_PTR(-EAGAIN)) {
> +			/*
> +			 * A non-zero page was skipped under
> +			 * only_merge_zero_pages: it was examined but not
> +			 * merged, so count it as scanned and keep going.
> +			 */

Similarly schloppy.

> +			ksm_pages_scanned++;
> +			continue;
> +		}
>  		cmp_and_merge_page(page, rmap_item);
>  		put_page(page);
>  		ksm_pages_scanned++;
> --
> 2.34.1
>

All in all adding far too much complexity in a cycle which is
fix-focused...

--
Cheers, Lorenzo