Message-ID: <ZjIo7E/IBYlNvO8H@e133380.arm.com>
Date: Wed, 1 May 2024 12:35:08 +0100
From: Dave Martin <Dave.Martin@....com>
To: Dorine Tipo <dorine.a.tipo@...il.com>
Cc: Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, Shuah Khan <shuah@...nel.org>,
	linux-arm-kernel@...ts.infradead.org,
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-kernel-mentees@...ts.linuxfoundation.org,
	Javier Carrasco <javier.carrasco.cruz@...il.com>
Subject: Re: [PATCH] selftests:arm64: Test PR_SVE_VL_INHERIT after a double
 fork

On Mon, Apr 29, 2024 at 04:40:12AM +0000, Dorine Tipo wrote:
> [PATCH] selftests:arm64: Test PR_SVE_VL_INHERIT after a double fork

Nit: Please follow the same commit message prefix convention as in the
existing code (e.g., try git log tools/testing/selftests/arm64/fp/ ).

[...]

> Add a new test, double_fork_test() to check the inheritance of the SVE
> vector length after a double fork.
> The `EXPECTED_TESTS` macro has been updated to account for this additional
> test.
> This patch addresses task 7 on the TODO list.
> 
> Signed-off-by: Dorine Tipo <dorine.a.tipo@...il.com>
> ---
>  tools/testing/selftests/arm64/fp/za-fork.c | 95 +++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/arm64/fp/za-fork.c b/tools/testing/selftests/arm64/fp/za-fork.c
> index 587b94648222..35229e570dcf 100644
> --- a/tools/testing/selftests/arm64/fp/za-fork.c
> +++ b/tools/testing/selftests/arm64/fp/za-fork.c
> @@ -11,7 +11,7 @@
> 
>  #include "kselftest.h"
> 
> -#define EXPECTED_TESTS 1
> +#define EXPECTED_TESTS 2
> 
>  int fork_test(void);
>  int verify_fork(void);
> @@ -69,6 +69,97 @@ int fork_test_c(void)
> 	}

"git am" or "patch" report the patch as corrupted here, any idea why?

Fixing the whitespace here allows the patch to be applied, but it still
doesn't seem to build, so I think there are bigger problems here.

>  }
> 
> +int double_fork_test(void)
> +{
> +	pid_t newpid, grandchild_pid, waiting;
> +	int ret, child_status, parent_result;
> +
> +	ret = prctl(PR_SVE_SET_VL, vl | PR_SVE_VL_INHERIT);
> +	if (ret < 0)
> +		ksft_exit_fail_msg("Failed to set SVE VL %d\n", vl);

Thanks for the attempt, but this file doesn't look like the correct
place to add these tests.  This file is specific to testing SME (the
"ZA" register array is specific to SME). Looking at the history of the
TODO file, it looks like the entry you're looking at was related to the
tests in vec-syscfg.c.  That looks like a better place to test the API
behaviour of these prctl calls; most of the needed framework is already
there.

So, please consider adapting vec-syscfg.c to add the double-fork tests,
rather than trying to write this from scratch.

(See commit e96595c55d23 ("kselftest/arm64: Add a TODO list for floating
point tests") for history.)


If you haven't already done so, you should go and look at the following
to get a better idea of how these prctl calls are supposed to work:

Documentation/arch/arm64/sve.rst
Documentation/arch/arm64/sme.rst

The prctl(2) man page also documents the SVE prctls:

https://man7.org/linux/man-pages/man2/prctl.2.html .


[more comments below]

> +
> +	newpid = fork();
> +	if (newpid == 0) {
> +		/* In child */
> +		if (!verify_fork()) {
> +			ksft_print_msg("ZA state invalid in child\n");
> +			exit(0);
> +		}
> +
> +		grandchild_pid = fork();
> +		if (grandchild_pid == 0) {
> +			/* in grandchild */
> +			if (!verfy_fork()) {
> +				ksft_print_msg("ZA state invalid in grandchild\n");
> +				exit(0);
> +			}
> +
> +			ret = prctl(PR_SVE_GET_VL);
> +			if (ret & PR_SVE_VL_INHERIT) {
> +				ksft_print_msg("prctl() reports _INHERIT\n");
> +				return;
> +			}
> +			 ksft_print_msg("prctl() does not report _INHERIT\n");
> +
> +		} else if (grandchild_pid < 0) {
> +			ksft_print_msg("fork() failed in first child: %d\n", grandchild_pid);
> +			return 0;
> +		}
> +
> +		/*  Wait for the grandchild process to exit */
> +		waiting = waitpid(grandchild_pid, &child_status, 0);
> +		if (waiting < 0) {
> +			if (errno == EINTR)
> +				continue;
> +			ksft_print_msg("waitpid() failed: %d\n", errno);
> +			return 0;
> +		}
> +		if (waiting != grandchild_pid) {
> +			ksft_print_msg("waitpid() returned wrong PID\n");
> +			return 0;
> +		}
> +
> +		if (!WIFEXITED(child_status)) {
> +			ksft_print_msg("grandchild did not exit\n");
> +			return 0;
> +		}
> +
> +		exit(1);
> +		}
> +	}
> +	if (newpid < 0) {
> +		ksft_print_msg("fork() failed: %d\n", newpid);
> +
> +		return 0;
> +	}
> +
> +	parent_result = verify_fork();
> +	if (!parent_result)
> +		ksft_print_msg("ZA state invalid in parent\n");
> +
> +	for (;;) {
> +		waiting = waitpid(newpid, &child_status, 0);
> +
> +		if (waiting < 0) {
> +			if (errno == EINTR)
> +				continue;
> +			ksft_print_msg("waitpid() failed: %d\n", errno);
> +			return 0;
> +		}
> +		if (waiting != newpid) {
> +			ksft_print_msg("waitpid() returned wrong PID\n");
> +			return 0;
> +		}
> +
> +		if (!WIFEXITED(child_status)) {
> +			ksft_print_msg("child did not exit\n");
> +			return 0;
> +		}
> +
> +		return WEXITSTATUS(child_status) && parent_result;
> +	}
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	int ret, i;
> @@ -86,11 +177,13 @@ int main(int argc, char **argv)
>  	ret = open("/proc/sys/abi/sme_default_vector_length", O_RDONLY, 0);
>  	if (ret >= 0) {
>  		ksft_test_result(fork_test(), "fork_test\n");
> +		ksft_test_result(double_fork_test(), "double_fork_test\n");
> 
>  	} else {
>  		ksft_print_msg("SME not supported\n");
>  		for (i = 0; i < EXPECTED_TESTS; i++) {
>  			ksft_test_result_skip("fork_test\n");
> +			ksft_test_result_skip("double_fork_test\n");

I'm not very familiar with the kselftest framework, but this looks
weird to me.

Why should we skip both tests twice when there are two tests?

(Looking at the history, I think this loop may have been left behind
from an old edit and perhaps shouldn't be there any more.  Comparing
with the way ksft_test_result_skip() is used in other tests may provide
a clue.  Maybe Mark Brown remembers...)


>  		}
>  	}

And finally... how are you running these tests?

Cheers
---Dave

Powered by blists - more mailing lists