Re: ksm: add mremap selftests for ksm_rmap_walk

[David Hildenbrand (Arm)]

On 4/7/26 08:08, xu.xin16@xxxxxxxxxx wrote:
> From: xu xin <xu.xin16@xxxxxxxxxx>
> 
> The existing tools/testing/selftests/mm/rmap.c has already one testcase
> for ksm_rmap_walk in TEST_F(migrate, ksm), which takes use of migration
> of page from one NUMA node to another NUMA node. However, it just lacks
> the senario of mremapped VMAs.
> 
> Before migrating, we add the calling of mremap() to address mapped with KSM
> pages, which is specailly to test a optimization which is introduced by this
> patch ("ksm: Optimize rmap_walk_ksm by passing a suitable address range")
> https://lore.kernel.org/all/20260212193045556CbzCX8p9gDu73tQ2nvHEI@xxxxxxxxxx/
> 
> Result:
> TAP version 13
> 1..5
> ok 1 migrate.anon
> ok 2 migrate.shm
> ok 3 migrate.file # SKIP Failed in worker
> ok 4 migrate.ksm
> ok 5 migrate.ksm_and_mremap
> 
> Signed-off-by: xu xin <xu.xin16@xxxxxxxxxx>
> ---
>  tools/testing/selftests/mm/rmap.c    | 69 ++++++++++++++++++++++++++++
>  tools/testing/selftests/mm/vm_util.c | 38 +++++++++++++++
>  tools/testing/selftests/mm/vm_util.h |  2 +
>  3 files changed, 109 insertions(+)
> 
> diff --git a/tools/testing/selftests/mm/rmap.c b/tools/testing/selftests/mm/rmap.c
> index 53f2058b0ef2..65470def2bf1 100644
> --- a/tools/testing/selftests/mm/rmap.c
> +++ b/tools/testing/selftests/mm/rmap.c
> @@ -430,4 +430,73 @@ TEST_F(migrate, ksm)
>  	propagate_children(_metadata, data);
>  }
> 
> +/* To test if ksm page can be migrated when it's mremapped */
> +int merge_mremap_and_migrate(struct global_data *data)
> +{
> +	int ret = 0;
> +	/* Allocate range and set the same data */
> +	data->mapsize = 3*getpagesize();
> +	data->region = mmap(NULL, data->mapsize, PROT_READ|PROT_WRITE,
> +			   MAP_PRIVATE|MAP_ANON, -1, 0);
> +	if (data->region == MAP_FAILED)
> +		ksft_exit_fail_perror("mmap failed");
> +
> +	memset(data->region, 0x77, data->mapsize);

What happens if you mremap() after faulting, but before merging?

rmap_item->address always holds the user space address of the entry in
the parent process. It must match the one in the child process, because
mremap() will unmerge/unshare in the child.

And it must match the one in the parent, as mremap() would similarly
unmerge/unshare.

Maybe doing the mremap() before merging (but after faulting) would
trigger what Hugh described.

break_cow() and friends don't care about the rmap, as they simply jump
directly to the user space address in the process.

In rmap_walk_ksm(), I think the concern Hugh raised is that we are using

	const pgoff_t pgoff = rmap_item->address >> PAGE_SHIFT;

but we'd actually need a pgoff into the anon_vma. Without mremap, it
does not matter, they are the same (tests keep passing). But with mremap
it's not longer the same.

I think one could store it in the ksm_rmap_item, but that would increase
it's size. It's essentially the folio->index of the original page we are
replacing.

Or we could just remember "pgoff is not that simple because mremap was
involved, so walk the whole damn thing".

We could also just try walking all involved processes, looking only at
that user space address (but that gets more tricky with rmap locking etc
...).

Anyhow, I think that's the concern Hugh raised, IIUC.

> +
> +	if (ksm_start() < 0)
> +		return FAIL_ON_CHECK;
> +
> +	/* 1  2 expected */
> +	ksft_print_msg("Shared: %ld (1 expected) Sharing: %ld (2 expected)\n",
> +		ksm_get_pages_shared(), ksm_get_pages_sharing());
> +
> +	/*
> +	 * Mremap the second pagesize address range into the third pagesize
> +	 * address.
> +	 */
> +	data->region = mremap(data->region + getpagesize(), getpagesize(), getpagesize(),
> +			 MREMAP_MAYMOVE|MREMAP_FIXED, data->region + 2*getpagesize());

There would not be a KSM page after this mremap(), no?


-- 
Cheers,

David