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

[David Hildenbrand <david@redhat.com>]

On 16.07.25 10:27, Wei Yang wrote:
> As David suggested, currently we don't have a high level test case to
> verify the behavior of rmap. This patch introduce the verification on
> rmap by migration.
> 
> The general idea is if migrate one shared page between processes, this
> would be reflected in all related processes. Otherwise, we have problem
> in rmap.
> 
> Currently it covers following four scenarios:
> 
>    * anonymous page
>    * shmem page
>    * pagecache page
>    * ksm page
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Cc: Rik van Riel <riel@surriel.com>
> Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Harry Yoo <harry.yoo@oracle.com>
> 
> ---
> v1: fault in region by just accessing it instead of print it
> ---
>   MAINTAINERS                               |   1 +
>   tools/testing/selftests/mm/.gitignore     |   1 +
>   tools/testing/selftests/mm/Makefile       |   3 +
>   tools/testing/selftests/mm/rmap.c         | 470 ++++++++++++++++++++++
>   tools/testing/selftests/mm/run_vmtests.sh |   4 +
>   5 files changed, 479 insertions(+)
>   create mode 100644 tools/testing/selftests/mm/rmap.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 70df1d8cfaf5..5fb62a275c37 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15917,6 +15917,7 @@ S:	Maintained
>   F:	include/linux/rmap.h
>   F:	mm/page_vma_mapped.c
>   F:	mm/rmap.c
> +F:	tools/testing/selftests/mm/rmap.c
>   
>   MEMORY MANAGEMENT - SECRETMEM
>   M:	Andrew Morton <akpm@linux-foundation.org>
> diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
> index f2dafa0b700b..c1f3de607ba2 100644
> --- a/tools/testing/selftests/mm/.gitignore
> +++ b/tools/testing/selftests/mm/.gitignore
> @@ -57,3 +57,4 @@ pkey_sighandler_tests_32
>   pkey_sighandler_tests_64
>   guard-regions
>   merge
> +rmap
> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
> index ae6f994d3add..c7e3a19b5555 100644
> --- a/tools/testing/selftests/mm/Makefile
> +++ b/tools/testing/selftests/mm/Makefile
> @@ -100,6 +100,7 @@ TEST_GEN_FILES += hugetlb_dio
>   TEST_GEN_FILES += droppable
>   TEST_GEN_FILES += guard-regions
>   TEST_GEN_FILES += merge
> +TEST_GEN_FILES += rmap
>   
>   ifneq ($(ARCH),arm64)
>   TEST_GEN_FILES += soft-dirty
> @@ -227,6 +228,8 @@ $(OUTPUT)/ksm_tests: LDLIBS += -lnuma
>   
>   $(OUTPUT)/migration: LDLIBS += -lnuma
>   
> +$(OUTPUT)/rmap: LDLIBS += -lnuma
> +
>   local_config.mk local_config.h: check_config.sh
>   	/bin/sh ./check_config.sh $(CC)
>   
> diff --git a/tools/testing/selftests/mm/rmap.c b/tools/testing/selftests/mm/rmap.c
> new file mode 100644
> index 000000000000..e05cd8a9c1bc
> --- /dev/null
> +++ b/tools/testing/selftests/mm/rmap.c
> @@ -0,0 +1,470 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RMAP functional tests
> + *
> + * Author(s): Wei Yang <richard.weiyang@gmail.com>
> + */
> +
> +#include "../kselftest_harness.h"
> +#include <strings.h>
> +#include <pthread.h>
> +#include <numa.h>
> +#include <numaif.h>
> +#include <sys/mman.h>
> +#include <sys/prctl.h>
> +#include <sys/types.h>
> +#include <signal.h>
> +#include <time.h>
> +#include <sys/sem.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +
> +#include "vm_util.h"
> +
> +#define TOTAL_LEVEL 5
> +#define MAX_CHILDREN 3
> +
> +#define FAIL_ON_CHECK	(1 << 0)
> +#define FAIL_ON_WORK	(1 << 1)
> +
> +struct sembuf sem_wait = {0, -1, 0};
> +struct sembuf sem_signal = {0, 1, 0};
> +
> +static const char initial_data[] = "Hello, world 0!";
> +static const char updated_data[] = "Hello, World 0!";
> +
> +enum backend_type {
> +	ANON,
> +	SHM,
> +	NORM_FILE,
> +};
> +
> +#define PREFIX "kst_rmap"
> +#define MAX_FILENAME_LEN 256
> +const char *suffixes[] = {
> +	"",
> +	"_shm",
> +	"_norm",
> +};
> +
> +struct global_data;
> +typedef int (*work_fn)(struct global_data *data);
> +typedef int (*check_fn)(struct global_data *data);
> +typedef void (*prepare_fn)(struct global_data *data);
> +
> +struct global_data {
> +	int worker_level;
> +
> +	int semid;
> +	int pipefd[2];
> +
> +	unsigned int mapsize;
> +	unsigned int rand_seed;
> +	char *region;
> +
> +	prepare_fn do_prepare;
> +	work_fn do_work;
> +	check_fn do_check;
> +
> +	enum backend_type backend;
> +	char filename[MAX_FILENAME_LEN];
> +
> +	unsigned long *expected_pfn;
> +};
> +
> +/*
> + * Create a process tree with TOTAL_LEVEL height and at most MAX_CHILDREN
> + * children for each.
> + *
> + * It will randomly select one process as 'worker' process which will
> + * 'do_work' until all processes are created. And all other processes will
> + * wait until 'worker' finish its work.
> + */
> +int propagate_children(struct global_data *data)
> +{
> +	pid_t pid;
> +	unsigned int num_child;
> +	int status;
> +	int ret = 0;
> +	int curr_child, worker_child;
> +	int curr_level = 1;
> +	bool is_worker = true;
> +
> +repeat:
> +	num_child = rand_r(&data->rand_seed) % MAX_CHILDREN + 1;
> +	worker_child = is_worker ? rand_r(&data->rand_seed) % num_child : -1;
> +
> +	for (curr_child = 0; curr_child < num_child; curr_child++) {
> +		pid = fork();
> +
> +		if (pid < 0) {
> +			perror("Error: fork\n");
> +		} else if (pid == 0) {
> +			curr_level++;
> +
> +			if (curr_child != worker_child)
> +				is_worker = false;
> +
> +			if (curr_level == TOTAL_LEVEL)
> +				break;
> +
> +			data->rand_seed += curr_child;
> +			goto repeat;
> +		}
> +	}
> +
> +	if (data->do_prepare)
> +		data->do_prepare(data);
> +
> +	close(data->pipefd[1]);
> +
> +	if (is_worker && curr_level == data->worker_level) {
> +		/* This is the worker process, first wait last process created */
> +		char buf;
> +
> +		while (read(data->pipefd[0], &buf, 1) > 0)
> +			;
> +
> +		if (data->do_work)
> +			ret = data->do_work(data);
> +
> +		/* Kick others */
> +		semctl(data->semid, 0, IPC_RMID);
> +	} else {
> +		/* Wait worker finish */
> +		semop(data->semid, &sem_wait, 1);
> +		if (data->do_check)
> +			ret = data->do_check(data);
> +	}
> +
> +	/* Wait all child to quit */
> +	while (wait(&status) > 0) {
> +		if (WIFEXITED(status))
> +			ret |= WEXITSTATUS(status);
> +	}
> +
> +	return ret;
> +}
> +
> +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?

> +	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.

> +	/* 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.

> +
> +	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.

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