Re: [PATCH v8 3/3] ksm: add mremap selftests for ksm_rmap_walk

[David Hildenbrand (Arm)]

On 6/9/26 06:47, 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 scenario of mremapped VMAs.
> 
> We add the calling of mremap() and then trigger KSM to merge pages before
> migrating, which is specifically to test an optimization which is
> introduced by this patch ("ksm: Optimize rmap_walk_ksm by passing a
> suitable address pgoff").
> 
> This test can reproduce the issue that Hugh points out at
> https://lore.kernel.org/all/02e1b8df-d568-8cbb-b8f6-46d5476d9d75@xxxxxxxxxx/
> 
> Signed-off-by: xu xin <xu.xin16@xxxxxxxxxx>
> ---
>  tools/testing/selftests/mm/rmap.c | 86 +++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/tools/testing/selftests/mm/rmap.c b/tools/testing/selftests/mm/rmap.c
> index 53f2058b0ef2..1cdc4beb48c2 100644
> --- a/tools/testing/selftests/mm/rmap.c
> +++ b/tools/testing/selftests/mm/rmap.c
> @@ -430,4 +430,90 @@ TEST_F(migrate, ksm)
>  	propagate_children(_metadata, data);
>  }
> 
> +static void prepare_pages(struct global_data *data, int nr_pages)
> +{
> +	/* Allocate exactly pages for the test */
> +	data->mapsize = nr_pages * 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");
> +
> +	/* Fill all pages with identical content to encourage KSM merging */
> +	memset(data->region, 0x77, data->mapsize);
> +}
> +
> +static int mremap_merge_and_migrate(struct global_data *data)
> +{
> +	int ret;
> +	void *old_region;
> +	void *new_region;
> +	int nr_pages = 32;

Likely you should do

const unsigned int nr_pages = 16;

and then simply multiply it by 2 for the temporary larger mmap.

> +	long merging_pages;
> +
> +	prepare_pages(data, nr_pages);

Is it really worth moving that into a helper function?

> +
> +	if (ksm_start() < 0)
> +		return FAIL_ON_CHECK;

Why the merge here? Wouldn't we want to merge after mremap()? mremap() will
just unshare either way?

> +
> +	old_region = data->region;
> +	/*
> +	 * Mremap the second half region to the first half location (FIXED).
> +	 */

Okay, so we create a VMA and populated anonymous pages. Might be from THPs, do
we care? I guess due to the split-on-dedup, this should be ok.

Now we mremap half of it over the other half.


> +	new_region = mremap(old_region + data->mapsize / 2, data->mapsize / 2,
> +			    data->mapsize / 2, MREMAP_MAYMOVE | MREMAP_FIXED,
> +			    old_region);
> +	if (new_region == MAP_FAILED) {
> +		ksft_print_msg("mremap failed: %s\n", strerror(errno));
> +		return FAIL_ON_CHECK;
> +	}
> +	data->region = new_region;
> +	data->mapsize /= 2;	/* mapping is now half of original */

Let's avoid the use of trailing comments where possible.

> +
> +	if (ksm_start() < 0)
> +		return FAIL_ON_CHECK;
> +
> +	/* Attempt to migrate the merged KSM page */
> +	ret = try_to_move_page(data->region);
> +	if (ret != 0) {
> +		ksft_print_msg("migration of KSM page after mremap failed\n");
> +		return FAIL_ON_CHECK;
> +	}
> +
> +	/* Ensure ksmd scan two turns at least to update ksm counters */
> +	if (ksm_start() < 0)
> +		return FAIL_ON_CHECK;

Why do we have to re-run KSM? ksm_start() already performed two runs. To remove
any leftover rmap entries?

But fundamentally, wouldn't it better to query before+after the
try_to_move_page() the PFN of all pages, to (a) make sure that they are the same
before (all merged, if not -> skip) and (b) make sure that all were migrated (if
still old PFN -> not migrated -> BUG).

> +
> +	merging_pages = ksm_get_self_merging_pages();
> +	printf("merging_pages:%ld\n", merging_pages);
> +	if (merging_pages != nr_pages / 2) {


[...]

> +TEST_F(migrate, ksm_and_mremap)
> +{
> +	struct global_data *data = &self->data;
> +	int ret;
> +
> +	/* Skip if KSM is not available */
> +	if (ksm_stop() < 0)
> +		SKIP(return, "accessing \"/sys/kernel/mm/ksm/run\" failed");
> +	if (ksm_get_full_scans() < 0)
> +		SKIP(return, "accessing \"/sys/kernel/mm/ksm/full_scan\" failed");
> +
> +	ret = prctl(PR_SET_MEMORY_MERGE, 1, 0, 0, 0);
> +	if (ret < 0 && errno == EINVAL)
> +		SKIP(return, "PR_SET_MEMORY_MERGE not supported");
> +	else if (ret)
> +		ksft_exit_fail_perror("PR_SET_MEMORY_MERGE=1 failed");

Why are we not simply using MADV_MERGEABLE instead of the prctl?

-- 
Cheers,

David