Re: [LTP] [Valgrind-developers] [PATCH] Make userfaultfd0{1, 3, 4} LTP tests valgrind compatible

[Petr Vorel <pvorel@suse.cz>]

Hi Martin,

[ Cc Ricardo ]

> Historically a page fault is handled in the linux kernel. The
> userfaultfd mechanism [1] was introduced in Linux 4.3 (2015) to allow
> custom handling page faults in userspace. It allows for custom page
> fault handling strategies such as lazy loading, live migration of qemu
> VMs, etc.  With userfaultfd, kernel communicates with userspace via a
> file descriptor (userfaultfd).  Userfaultfd LTP tests cover this.

> These tests set up a main thread and a page fault handler thread.  Once
> a page fault happens in the main thread, kernel stops it, and let's the
> handler take care of the page fault.

> This is a problem for Valgrind, because it serializes the threads
> execution (using one "big mutex", VG_(acquire_BigLock)).  Once kernel
> stops the main thread, under valgrind, the handler thread doesn't have
> a chance to handle the page fault.  Valgrind stalls till the timeout
> (adjustable via LTP_TIMEOUT_MUL ;)

> However, some of the testcases can be easily changed to use forked
> processes instead of threads.  That's what this patch does.  When
> client program forks, Valgrind forks too, and that allows for the needed
> parallelism to handle the page fault.

You understand process vs. threads more than me. But shouldn't mmap() use
MAP_SHARED instead of MAP_PRIVATE for those which aren't using /dev/userfaultfd?

> This redesign can't be easily done with some of the tests, such as e.g.
> userfaultfd02 where both the main thread and the handler thread need to
> share the address space.  That could be done via clone() and is proven

What is the difference with userfaultfd02 then?

> to work.  But unfortunately there is another Valgrind limitation in clone
> flags it supports, so this also isn't a practical way to go.

> That said, this update tweaks userfaultfd01, userfaultfd03, and
> userfaultfd04 in a way that they become "compatible" with Valgrind.
> It's a test coverage improvement.

Also doc should be updated (mentions thread).

Please don't forget to add it next time:
Signed-off-by: Martin Cermak <mcermak@redhat.com>

> [1] https://www.kernel.org/doc/html/latest/admin-guide/mm/userfaultfd.html
> ---
>  .../kernel/syscalls/userfaultfd/userfaultfd01.c   | 13 +++++++++----
>  .../kernel/syscalls/userfaultfd/userfaultfd03.c   | 15 ++++++++++-----
>  .../kernel/syscalls/userfaultfd/userfaultfd04.c   | 13 +++++++++----
>  3 files changed, 28 insertions(+), 13 deletions(-)

> diff --git a/testcases/kernel/syscalls/userfaultfd/userfaultfd01.c b/testcases/kernel/syscalls/userfaultfd/userfaultfd01.c
> index 7368d3863..c927cda95 100644
> --- a/testcases/kernel/syscalls/userfaultfd/userfaultfd01.c
> +++ b/testcases/kernel/syscalls/userfaultfd/userfaultfd01.c
> @@ -57,7 +57,7 @@ static void reset_pages(void)
>  	SAFE_MUNMAP(copy_page, page_size);
>  }

> -static void *handle_thread(void)
> +static void *pagefault_handler(void)
>  {
>  	static struct uffd_msg msg;
>  	struct uffdio_copy uffdio_copy = {};
> @@ -91,7 +91,7 @@ static void *handle_thread(void)

>  static void run(unsigned int i)
>  {
> -	pthread_t thr;
> +	pid_t pid;
>  	struct uffdio_api uffdio_api = {};
>  	struct uffdio_register uffdio_register;
>  	struct tcase *tc = &tcases[i];
> @@ -112,7 +112,11 @@ static void run(unsigned int i)

>  	SAFE_IOCTL(uffd, UFFDIO_REGISTER, &uffdio_register);

> -	SAFE_PTHREAD_CREATE(&thr, NULL, (void *) handle_thread, NULL);
> +	pid = SAFE_FORK();
> +	if (pid == 0) {
> +		pagefault_handler();
> +		_exit(0);
> +	}

>  	char c = page[0xf];

> @@ -121,7 +125,7 @@ static void run(unsigned int i)
>  	else
>  		tst_res(TFAIL, "Pagefault not handled!");

> -	SAFE_PTHREAD_JOIN(thr, NULL);
> +	SAFE_WAITPID(pid, NULL, 0);
>  	reset_pages();
>  }

> @@ -129,4 +133,5 @@ static struct tst_test test = {
>  	.setup = setup,
>  	.test = run,
>  	.tcnt = ARRAY_SIZE(tcases),
> +	.forks_child = 1,
>  };
> diff --git a/testcases/kernel/syscalls/userfaultfd/userfaultfd03.c b/testcases/kernel/syscalls/userfaultfd/userfaultfd03.c
> index b65f39eca..f5d3be1ae 100644
> --- a/testcases/kernel/syscalls/userfaultfd/userfaultfd03.c
> +++ b/testcases/kernel/syscalls/userfaultfd/userfaultfd03.c
> @@ -61,7 +61,7 @@ static void reset_pages(void)
>  	SAFE_MUNMAP(copy_page, page_size);
>  }

> -static void *handle_thread(void)
> +static void *pagefault_handler(void)
>  {
>  	static struct uffd_msg msg;
>  	struct uffdio_copy uffdio_copy = {};
> @@ -95,7 +95,7 @@ static void *handle_thread(void)

>  static void run(void)
>  {
> -	pthread_t thr;
> +	pid_t pid;
>  	struct uffdio_api uffdio_api = {};
>  	struct uffdio_register uffdio_register;

> @@ -112,7 +112,11 @@ static void run(void)

>  	SAFE_IOCTL(uffd, UFFDIO_REGISTER, &uffdio_register);

> -	SAFE_PTHREAD_CREATE(&thr, NULL, (void *) handle_thread, NULL);
> +	pid = SAFE_FORK();
> +		if (pid == 0) {
> +		pagefault_handler();
> +		_exit(0);
> +	}

>  	char c = page[0xf];

> @@ -121,7 +125,7 @@ static void run(void)
>  	else
>  		tst_res(TFAIL, "Pagefault not handled via /dev/userfaultfd");

> -	SAFE_PTHREAD_JOIN(thr, NULL);
> +	SAFE_WAITPID(pid, NULL, 0);
>  	reset_pages();
>  }

> @@ -132,5 +136,6 @@ static struct tst_test test = {
>  	.needs_kconfigs = (const char *[]) {
>  		"CONFIG_USERFAULTFD=y",
>  		NULL
> -	}
> +	},
> +	.forks_child = 1,
>  };
> diff --git a/testcases/kernel/syscalls/userfaultfd/userfaultfd04.c b/testcases/kernel/syscalls/userfaultfd/userfaultfd04.c
> index 4eb811e45..13883a711 100644
> --- a/testcases/kernel/syscalls/userfaultfd/userfaultfd04.c
> +++ b/testcases/kernel/syscalls/userfaultfd/userfaultfd04.c
> @@ -33,7 +33,7 @@ static void reset_pages(void)
>  	SAFE_MUNMAP(page, page_size);
>  }

> -static void *handle_thread(void)
> +static void *pagefault_handler(void)
>  {
>  	static struct uffd_msg msg;
>  	struct uffdio_zeropage uffdio_zeropage = {};
> @@ -64,7 +64,7 @@ static void *handle_thread(void)

>  static void run(void)
>  {
> -	pthread_t thr;
> +	pid_t pid;
>  	struct uffdio_api uffdio_api = {};
>  	struct uffdio_register uffdio_register;

> @@ -81,7 +81,11 @@ static void run(void)

>  	SAFE_IOCTL(uffd, UFFDIO_REGISTER, &uffdio_register);

> -	SAFE_PTHREAD_CREATE(&thr, NULL, (void *) handle_thread, NULL);
> +	pid = SAFE_FORK();
> +	if (pid == 0) {
> +		pagefault_handler();
> +		_exit(0);
> +	}

>  	for (int i = 0; i < page_size; i++) {
>  		if (page[i] != 0) {
> @@ -92,10 +96,11 @@ static void run(void)

>  	tst_res(TPASS, "Pagefault handled with UFFDIO_ZEROPAGE");

> -	SAFE_PTHREAD_JOIN(thr, NULL);
> +	SAFE_WAITPID(pid, NULL, 0);
>  	reset_pages();
>  }

>  static struct tst_test test = {
>  	.test_all = run,
> +	.forks_child = 1,
>  };

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp