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

[David Hildenbrand (Arm)]

On 6/29/26 11:44, 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 | 107 ++++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> diff --git a/tools/testing/selftests/mm/rmap.c b/tools/testing/selftests/mm/rmap.c
> index 53f2058b0ef2..ad731bdff4fa 100644
> --- a/tools/testing/selftests/mm/rmap.c
> +++ b/tools/testing/selftests/mm/rmap.c
> @@ -430,4 +430,111 @@ TEST_F(migrate, ksm)
>  	propagate_children(_metadata, data);
>  }
> 
> +/*
> + * Check if All PFNs of the region are the same to the input pfn.
> + *
> + * @pagemap_fd: file descriptor of /proc/pid/pagemap.
> + * @region: the start address of the associated region.
> + * @nr_pages: the number of pages that the region contains.
> + * @pfn: the referenced PFN.
> + */
> +static bool merged_to_pfn(int pagemap_fd, void *region, int nr_pages,
> +		unsigned long pfn)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_pages; i++)
> +		if (pagemap_get_pfn(pagemap_fd, region + i * getpagesize()) != pfn)
> +			return false;
> +
> +	return true;
> +}
> +
> +static int mremap_merge_and_migrate(struct global_data *data)
> +{
> +	int ret, pagemap_fd;
> +	void *old_region;
> +	void *new_region;
> +	int nr_pages = 32;
> +	unsigned long old_pfn;
> +
> +	/* 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");
> +	memset(data->region, 0x77, data->mapsize);
> +
> +	/*
> +	 * Mremap the second half region to the first half location (FIXED).
> +	 */
> +	old_region = data->region;
> +	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;
> +
> +	/* madvise MADV_MERGABLE and merge these pages */
> +	madvise(data->region, data->mapsize, MADV_MERGEABLE);
> +	if (ksm_start() < 0)
> +		return FAIL_ON_WORK;
> +
> +	pagemap_fd = open("/proc/self/pagemap", O_RDONLY);

I think there are various improvements and simplifications we can perform. In particular,
 I don't think we need the errors messages or use data-> members.

What about the following simplification, to move this over the finishing line? (untested)

There is the low chance of page compaction migrating the page while we check for it. Not sure 
if we should handle it (but it would involve retrying on PFN mismatch).


diff --git a/tools/testing/selftests/mm/rmap.c b/tools/testing/selftests/mm/rmap.c
index 53f2058b0ef2b..1b7ab46a520cf 100644
--- a/tools/testing/selftests/mm/rmap.c
+++ b/tools/testing/selftests/mm/rmap.c
@@ -430,4 +430,68 @@ TEST_F(migrate, ksm)
 	propagate_children(_metadata, data);
 }
 
+static bool range_maps_pfn(int pagemap_fd, void *region, int nr_pages,
+		unsigned long pfn)
+{
+	int i;
+
+	for (i = 0; i < nr_pages; i++)
+		if (pagemap_get_pfn(pagemap_fd, region + i * getpagesize()) != pfn)
+			return false;
+	return true;
+}
+
+TEST_F(migrate, ksm_and_mremap)
+{
+	unsigned long old_pfn, new_pfn;
+	void *region, *mremap_region;
+	const int nr_pages = 16;
+	size_t mmap_size;
+	int pagemap_fd;
+
+	/* 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");
+
+	pagemap_fd = open("/proc/self/pagemap", O_RDONLY);
+	if (pagemap_fd < 0)
+		SKIP(return, "opening pagemap failed");
+
+	/* Allocate and populate twice the anon pages initially. */
+	mmap_size = 2 * nr_pages * getpagesize();
+	region = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE,
+		      MAP_PRIVATE | MAP_ANON, -1, 0);
+	ASSERT_NE(region, MAP_FAILED);
+	memset(region, 0x77, mmap_size);
+
+	/* mremap the second half over the first half, to stress rmap handling */
+	mmap_size /= 2;
+	mremap_region = mremap(region + mmap_size, mmap_size, mmap_size,
+			       MREMAP_MAYMOVE | MREMAP_FIXED, region);
+	ASSERT_EQ(mremap_region, region);
+
+	/* Merge all pages into a single KSM page. */
+	madvise(region, mmap_size, MADV_MERGEABLE);
+	ASSERT_EQ(ksm_start(), 0);
+
+	/* The whole range should map the same KSM page. */
+	old_pfn = pagemap_get_pfn(pagemap_fd, region);
+	if (old_pfn == -1ul)
+		SKIP(return, "Obtaining PFN failed");
+	ASSERT_TRUE(range_maps_pfn(pagemap_fd, region, nr_pages, old_pfn));
+
+	/*
+	 * Migrate the KSM page; the whole range should map the new (migrated)
+	 * KSM page.
+	 */
+	ASSERT_EQ(try_to_move_page(region), 0);
+	new_pfn = pagemap_get_pfn(pagemap_fd, region);
+	if (new_pfn == -1ul)
+		SKIP(return, "Obtaining PFN failed");
+	ASSERT_NE(new_pfn, old_pfn);
+	ASSERT_TRUE(range_maps_pfn(pagemap_fd, region, nr_pages, new_pfn));
+}
+
 TEST_HARNESS_MAIN
-- 
2.43.0

-- 
Cheers,

David