[PATCH 3/3] selftests/powerpc: Don't rely on segfault to rerun the test

[Gustavo Romero]

On 01/16/2020 07:05 PM, Gustavo Luiz Duarte wrote:
> The test case tm-signal-context-force-tm expects a segfault to happen on
> returning from signal handler, and then does a setcontext() to run the test
> again. However, the test doesn't always segfault, causing the test to run a
> single time.
> 
> This patch fixes the test by putting it within a loop and jumping, via
> setcontext, just prior to the loop in case it segfaults. This way we get the
> desired behavior (run the test COUNT_MAX times) regardless if it segfaults or
> not. This also reduces the use of setcontext for control flow logic, keeping it
> only in the segfault handler.
> 
> Also, since 'count' is changed within the signal handler, it is declared as
> volatile to prevent any compiler optimization getting confused with
> asynchronous changes.
> 
> Signed-off-by: Gustavo Luiz Duarte <gustavold at linux.ibm.com>
> ---
>   .../powerpc/tm/tm-signal-context-force-tm.c   | 79 +++++++++----------
>   1 file changed, 37 insertions(+), 42 deletions(-)
> 
> diff --git a/tools/testing/selftests/powerpc/tm/tm-signal-context-force-tm.c b/tools/testing/selftests/powerpc/tm/tm-signal-context-force-tm.c
> index 31717625f318..9ff7bdb6d47a 100644
> --- a/tools/testing/selftests/powerpc/tm/tm-signal-context-force-tm.c
> +++ b/tools/testing/selftests/powerpc/tm/tm-signal-context-force-tm.c
> @@ -42,9 +42,10 @@
>   #endif
>   
>   /* Setting contexts because the test will crash and we want to recover */
> -ucontext_t init_context, main_context;
> +ucontext_t init_context;
>   
> -static int count, first_time;
> +/* count is changed in the signal handler, so it must be volatile */
> +static volatile int count;
>   
>   void usr_signal_handler(int signo, siginfo_t *si, void *uc)
>   {
> @@ -98,11 +99,6 @@ void usr_signal_handler(int signo, siginfo_t *si, void *uc)
>   
>   void seg_signal_handler(int signo, siginfo_t *si, void *uc)
>   {
> -	if (count == COUNT_MAX) {
> -		/* Return to tm_signal_force_msr() and exit */
> -		setcontext(&main_context);
> -	}
> -
>   	count++;
>   
>   	/* Reexecute the test */
> @@ -126,37 +122,40 @@ void tm_trap_test(void)
>   	 */
>   	getcontext(&init_context);
>   
> -	/* Allocated an alternative signal stack area */
> -	ss.ss_sp = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
> -			MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
> -	ss.ss_size = SIGSTKSZ;
> -	ss.ss_flags = 0;
> -
> -	if (ss.ss_sp == (void *)-1) {
> -		perror("mmap error\n");
> -		exit(-1);
> -	}
> -
> -	/* Force the allocation through a page fault */
> -	if (madvise(ss.ss_sp, SIGSTKSZ, MADV_DONTNEED)) {
> -		perror("madvise\n");
> -		exit(-1);
> -	}
> -
> -	/* Setting an alternative stack to generate a page fault when
> -	 * the signal is raised.
> -	 */
> -	if (sigaltstack(&ss, NULL)) {
> -		perror("sigaltstack\n");
> -		exit(-1);
> +	while (count < COUNT_MAX) {
> +		/* Allocated an alternative signal stack area */
> +		ss.ss_sp = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
> +				MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
> +		ss.ss_size = SIGSTKSZ;
> +		ss.ss_flags = 0;
> +
> +		if (ss.ss_sp == (void *)-1) {
> +			perror("mmap error\n");
> +			exit(-1);
> +		}
> +
> +		/* Force the allocation through a page fault */
> +		if (madvise(ss.ss_sp, SIGSTKSZ, MADV_DONTNEED)) {
> +			perror("madvise\n");
> +			exit(-1);
> +		}
> +
> +		/* Setting an alternative stack to generate a page fault when
> +		 * the signal is raised.
> +		 */
> +		if (sigaltstack(&ss, NULL)) {
> +			perror("sigaltstack\n");
> +			exit(-1);
> +		}
> +
> +		/* The signal handler will enable MSR_TS */
> +		sigaction(SIGUSR1, &usr_sa, NULL);
> +		/* If it does not crash, it might segfault, avoid it to retest */
> +		sigaction(SIGSEGV, &seg_sa, NULL);
> +
> +		raise(SIGUSR1);
> +		count++;
>   	}
> -
> -	/* The signal handler will enable MSR_TS */
> -	sigaction(SIGUSR1, &usr_sa, NULL);
> -	/* If it does not crash, it will segfault, avoid it to retest */
> -	sigaction(SIGSEGV, &seg_sa, NULL);
> -
> -	raise(SIGUSR1);
>   }
>   
>   int tm_signal_context_force_tm(void)
> @@ -169,11 +168,7 @@ int tm_signal_context_force_tm(void)
>   	 */
>   	SKIP_IF(!is_ppc64le());
>   
> -	/* Will get back here after COUNT_MAX interactions */
> -	getcontext(&main_context);
> -
> -	if (!first_time++)
> -		tm_trap_test();
> +	tm_trap_test();
>   
>   	return EXIT_SUCCESS;
>   }
> 

Reviewed-by: Gustavo Romero <gromero at linux.ibm.com>


Best regards,
Gustavo