Re: [PATCH 3/3] selftests/mm: assert rmap behave as expected

[Wei Yang <richard.weiyang@gmail.com>]

On Wed, Jul 16, 2025 at 03:34:11PM +0200, David Hildenbrand wrote:
>On 16.07.25 10:27, Wei Yang wrote:
[..]
>> +
>> +FIXTURE(migrate)
>> +{
>> +	struct global_data data;
>> +};
>> +
>> +FIXTURE_SETUP(migrate)
>> +{
>> +	struct global_data *data = &self->data;
>> +
>> +	ASSERT_EQ(numa_available(), 0);
>
>if (numa_available() < 0)
>	SKIP(return, "NUMA not available");
>
>Should that be a skip instead?
>

You are right, will fix it.

>> +	if (numa_bitmask_weight(numa_all_nodes_ptr) <= 1)
>> +		SKIP(return, "Not enough NUMA nodes available");
>> +
>> +	data->mapsize = getpagesize();> +
>> +	/* Prepare semaphore */
>> +	data->semid = semget(IPC_PRIVATE, 1, 0666 | IPC_CREAT);
>> +	ASSERT_NE(data->semid, -1);
>> +	ASSERT_NE(semctl(data->semid, 0, SETVAL, 0), -1);
>> +
>> +	/* Prepare pipe */
>> +	ASSERT_NE(pipe(data->pipefd), -1);
>> +
>> +	data->rand_seed = time(NULL);
>> +	srand(data->rand_seed);
>> +
>> +	data->worker_level = rand() % TOTAL_LEVEL + 1;
>> +
>> +	data->do_prepare = NULL;
>> +	data->do_work = NULL;
>> +	data->do_check = NULL;
>> +
>> +	data->backend = ANON;
>> +};
>> +
>> +FIXTURE_TEARDOWN(migrate)
>> +{
>> +	struct global_data *data = &self->data;
>> +
>> +	if (data->region != MAP_FAILED)
>> +		munmap(data->region, data->mapsize);
>> +	data->region = MAP_FAILED;
>> +	if (data->expected_pfn != MAP_FAILED)
>> +		munmap(data->expected_pfn, sizeof(unsigned long));
>> +	data->expected_pfn = MAP_FAILED;
>> +	semctl(data->semid, 0, IPC_RMID);
>> +	data->semid = -1;
>> +
>> +	close(data->pipefd[0]);
>> +
>> +	data->do_work = NULL;
>> +	data->do_check = NULL;
>> +
>> +	switch (data->backend) {
>> +	case ANON:
>> +		break;
>> +	case SHM:
>> +		shm_unlink(data->filename);
>> +		break;
>> +	case NORM_FILE:
>> +		unlink(data->filename);
>> +		break;
>> +	}
>> +}
>> +
>> +int try_to_move_page(char *region)
>> +{
>> +	int ret;
>> +	int node;
>> +	int status = 0;
>> +	volatile unsigned long dummy = 0;
>> +
>> +	/*
>> +	 * Fault in page in case it is not, otherwise move_pages() would
>> +	 * return -ENOENT.
>> +	 */
>> +	dummy = *((unsigned long *)region);
>
>Use FORCE_READ() here
>
>https://lkml.kernel.org/r/20250716123126.3851-1-lianux.mm@gmail.com
>
>But, this really must happen in all children before actually performing the
>move in the worker. Otherwise the other processes don't map the page and will
>just ... fault it in later.
>

Ok, will access the region in all child before migrate.

>> +	/* Prevent the compiler from optimizing out the entire loop: */
>> +	asm volatile("" : "+r" (dummy));
>> +
>> +	ret = move_pages(0, 1, (void **)&region, NULL, &status, MPOL_MF_MOVE_ALL);
>> +	if (ret != 0)
>> +		return FAIL_ON_WORK;
>> +
>> +	/* Pick up a different target node */
>> +	for (node = 0; node <= numa_max_node(); node++) {
>> +		if (numa_bitmask_isbitset(numa_all_nodes_ptr, node) && node != status)
>> +			break;
>> +	}
>> +
>> +	if (node > numa_max_node()) {
>> +		ksft_print_msg("Couldn't find available numa node for testing\n");
>> +		return FAIL_ON_WORK;
>> +	}
>> +
>> +	ret = move_pages(0, 1, (void **)&region, &node, &status, MPOL_MF_MOVE_ALL);
>> +	if (ret != 0)
>> +		return FAIL_ON_WORK;
>
>Probably, if we don't manage to migrate, we should retry a couple of times
>and then SKIP.
>
>Point is, migration might fail for various reasons (e.g., 2 NUMA nodes but
>one of them doesn't even have memory) etc.
>
>Migration failures might indicate other problems, yes, but false failures
>from the test are suboptimal.
>

Will add retry logic.

>> +
>> +	return 0;
>> +}
>> +
>> +int move_and_update(struct global_data *data)
>> +{
>> +	int ret;
>> +
>> +	ret = try_to_move_page(data->region);
>> +	if (ret != 0)
>> +		return ret;
>> +
>> +	/* Change the content */
>> +	strcpy(data->region, updated_data);
>> +
>> +	return ret;
>> +}
>> +
>> +int data_updated(struct global_data *data)
>> +{
>> +	if (data->region == MAP_FAILED)
>> +		return 0;
>> +
>> +	if (strncmp((char *)data->region, updated_data, strlen(updated_data)))
>> +		return FAIL_ON_CHECK;
>> +	return 0;
>> +}
>
>I assume checking the PFN is sufficient. No need for the additional data
>content. In particular, with proper anon pages (CoW, see below) that doesn't
>work either way.
>
>> +
>> +TEST_F(migrate, anon)
>> +{
>> +	pid_t root_pid;
>> +	int ret;
>> +	struct global_data *data = &self->data;
>> +
>> +	/* Map a shared area and fault in */
>> +	data->region = mmap(0, data->mapsize, PROT_READ | PROT_WRITE,
>> +				MAP_SHARED | MAP_ANONYMOUS, -1, 0);
>
>That is anon_shmem. We should test proper anon memory (MAP_PRIVATE), whereby
>pages are shared using CoW.
>

So the case should be 

  * mapping (MAP_PRIVATE | MAP_ANONYMOUS)
  * write some content in root parent
  * fault in for each child
  * do migration and record pfn
  * then check pfn is the same in each child

>anon_shmem should behave mostly like shmem.
>
>> +	ASSERT_NE(data->region, MAP_FAILED);
>> +	strcpy(data->region, initial_data);
>> +
>> +	data->do_work = move_and_update;
>> +	data->do_check = data_updated;
>> +
>> +	root_pid = getpid();
>> +
>> +	ret = propagate_children(data);
>> +
>> +	if (getpid() == root_pid) {
>> +		if (ret & FAIL_ON_WORK)
>> +			SKIP(return, "Failed on moving page");
>> +
>> +		ASSERT_EQ(ret, 0);
>> +	} else {
>> +		exit(ret);
>> +	}
>> +}
>
>[...]
>
>> +TEST_HARNESS_MAIN
>> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
>> index a38c984103ce..f6fb8dec6e64 100755
>> --- a/tools/testing/selftests/mm/run_vmtests.sh
>> +++ b/tools/testing/selftests/mm/run_vmtests.sh
>> @@ -83,6 +83,8 @@ separated by spaces:
>>   	test handling of page fragment allocation and freeing
>>   - vma_merge
>>   	test VMA merge cases behave as expected
>> +- rmap
>> +	test rmap behave as expected
>
>"behaves"
>
>-- 
>Cheers,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me