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

[David Hildenbrand (Arm)]

On 6/11/26 15:22, 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>

No need to resend before the merge window closes. So please resend after -rc1.

> ---

[...]

> +	/* Before migrating, check if All pages's PFN are the same */
> +	*data->expected_pfn = pagemap_get_pfn(pagemap_fd, data->region);

Should we check for valid pfns? (!= -1ul) and fail stating that we failed to
obtain a PFN?

> +	for (i = 1; i < nr_pages / 2; i++) {
> +		if (pagemap_get_pfn(pagemap_fd, data->region + i * page_size)
> +			!= *data->expected_pfn) {
> +			ksft_print_msg("PFN is not expected\n");
> +			return FAIL_ON_CHECK;
> +		}
> +	}
> +	old_pfn = *data->expected_pfn;
> +
> +	/* Attempt to migrate the merged KSM page */
> +	ret = try_to_move_page(data->region);
> +	if (ret != 0) {

if (ret)

?

> +		ksft_print_msg("migration of KSM page after mremap failed\n");
> +		return FAIL_ON_CHECK;
> +	}
> +
> +	/* After migrating, check if all PFN aren't the old */

Given that this test can really only succeed as root, we should always obtain
valid PFNs here.

Can we have a simple

static bool merged_to_pfn(int pagemap_fd, void *region, int nr_pages,
		unsigned long pfn)
{

}

Where we simply make sure that all PFNs are the same.

Then we can reuse that helper twice and simply once make sure that the first PFN
is different (and all other match that).

> +	for (i = 1; i < nr_pages / 2; i++) {
> +		if (pagemap_get_pfn(pagemap_fd, data->region + i * page_size)
> +			== old_pfn) {
> +			ksft_print_msg("Bug migration: still old PFN\n");
> +			return FAIL_ON_CHECK;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +
> +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");

As discussed I think it's probably better to just MADV_MERGABLE after the
mremap. That should be around everywhere, so no need for error handling.

> +
> +	ASSERT_EQ(mremap_merge_and_migrate(data), 0);
> +}
> +
>  TEST_HARNESS_MAIN


-- 
Cheers,

David