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

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

On Thu, Jul 17, 2025 at 03:17:48AM +0000, Wei Yang wrote:
>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
>

Hi, David

Is my understanding correct? If so, may I send a new version?

-- 
Wei Yang
Help you, Help me