[PATCH 1/2] selftests/powerpc: Check for pthread errors in tm-unavailable

[PATCH 1/2] selftests/powerpc: Check for pthread errors in tm-unavailable

[Cyril Bur]

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 .../testing/selftests/powerpc/tm/tm-unavailable.c  | 43 +++++++++++++++++-----
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
index 96c37f84ce54..e6a0fad2bfd0 100644
--- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c
+++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
@@ -15,6 +15,7 @@
  */
 
 #define _GNU_SOURCE
+#include <error.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -33,6 +34,11 @@
 #define VSX_UNA_EXCEPTION	2
 
 #define NUM_EXCEPTIONS		3
+#define err_at_line(status, errnum, format, ...) \
+	error_at_line(status, errnum,  __FILE__, __LINE__, format ##__VA_ARGS__)
+
+#define pr_warn(code, format, ...) err_at_line(0, code, format, ##__VA_ARGS__)
+#define pr_err(code, format, ...) err_at_line(1, code, format, ##__VA_ARGS__)
 
 struct Flags {
 	int touch_fp;
@@ -303,10 +309,19 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
 	 * checking if the failure cause is the one we expect.
 	 */
 	do {
+		int rc;
+
 		/* Bind 'ping' to CPU 0, as specified in 'attr'. */
-		pthread_create(&t0, attr, ping, (void *) &flags);
-		pthread_setname_np(t0, "ping");
-		pthread_join(t0, &ret_value);
+		rc = pthread_create(&t0, attr, ping, (void *) &flags);
+		if (rc)
+			pr_err(rc, "pthread_create()");
+		rc = pthread_setname_np(t0, "ping");
+		if (rc)
+			pr_warn(rc, "pthread_setname_np");
+		rc = pthread_join(t0, &ret_value);
+		if (rc)
+			pr_err(rc, "pthread_join");
+
 		retries--;
 	} while (ret_value != NULL && retries);
 
@@ -320,7 +335,7 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
 
 int main(int argc, char **argv)
 {
-	int exception; /* FP = 0, VEC = 1, VSX = 2 */
+	int rc, exception; /* FP = 0, VEC = 1, VSX = 2 */
 	pthread_t t1;
 	pthread_attr_t attr;
 	cpu_set_t cpuset;
@@ -330,13 +345,23 @@ int main(int argc, char **argv)
 	CPU_SET(0, &cpuset);
 
 	/* Init pthread attribute. */
-	pthread_attr_init(&attr);
+	rc = pthread_attr_init(&attr);
+	if (rc)
+		pr_err(rc, "pthread_attr_init()");
 
 	/* Set CPU 0 mask into the pthread attribute. */
-	pthread_attr_setaffinity_np(&attr, sizeof(cpu_set_t), &cpuset);
-
-	pthread_create(&t1, &attr /* Bind 'pong' to CPU 0 */, pong, NULL);
-	pthread_setname_np(t1, "pong"); /* Name it for systemtap convenience */
+	rc = pthread_attr_setaffinity_np(&attr, sizeof(cpu_set_t), &cpuset);
+	if (rc)
+		pr_err(rc, "pthread_attr_setaffinity_np()");
+
+	rc = pthread_create(&t1, &attr /* Bind 'pong' to CPU 0 */, pong, NULL);
+	if (rc)
+		pr_err(rc, "pthread_create()");
+
+	/* Name it for systemtap convenience */
+	rc = pthread_setname_np(t1, "pong");
+	if (rc)
+		pr_warn(rc, "pthread_create()");
 
 	flags.result = 0;
 
-- 
2.15.0

[PATCH 2/2] selftests/powerpc: Calculate spin time in tm-unavailable

[Cyril Bur]

Currently the tm-unavailable test spins for a fixed amount of time in
an attempt to ensure the FPU/VMX/VSX facilities are off. This value was
experimentally tested to be long enough.

Problems may arise if kernel heuristics were to change. This patch
should future proof this test.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
Because the test no longer needs to use such a conservative time for
the busy wait, it actually runs much faster.


 .../testing/selftests/powerpc/tm/tm-unavailable.c  | 92 ++++++++++++++++++++--
 1 file changed, 84 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
index e6a0fad2bfd0..54aeb7a7fbb1 100644
--- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c
+++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
@@ -33,6 +33,11 @@
 #define VEC_UNA_EXCEPTION	1
 #define VSX_UNA_EXCEPTION	2
 
+#define ERR_RETRY 1
+#define ERR_ADJUST 2
+
+#define COUNTER_INCREMENT (0x1000000)
+
 #define NUM_EXCEPTIONS		3
 #define err_at_line(status, errnum, format, ...) \
 	error_at_line(status, errnum,  __FILE__, __LINE__, format ##__VA_ARGS__)
@@ -45,6 +50,7 @@ struct Flags {
 	int touch_vec;
 	int result;
 	int exception;
+	uint64_t counter;
 } flags;
 
 bool expecting_failure(void)
@@ -87,14 +93,12 @@ void *ping(void *input)
 	 * Expected values for vs0 and vs32 after a TM failure. They must never
 	 * change, otherwise they got corrupted.
 	 */
+	long rc = 0;
 	uint64_t high_vs0 = 0x5555555555555555;
 	uint64_t low_vs0 = 0xffffffffffffffff;
 	uint64_t high_vs32 = 0x5555555555555555;
 	uint64_t low_vs32 = 0xffffffffffffffff;
 
-	/* Counter for busy wait */
-	uint64_t counter = 0x1ff000000;
-
 	/*
 	 * Variable to keep a copy of CR register content taken just after we
 	 * leave the transactional state.
@@ -217,7 +221,7 @@ void *ping(void *input)
 		  [ex_fp]     "i"  (FP_UNA_EXCEPTION),
 		  [ex_vec]    "i"  (VEC_UNA_EXCEPTION),
 		  [ex_vsx]    "i"  (VSX_UNA_EXCEPTION),
-		  [counter]   "r"  (counter)
+		  [counter]   "r"  (flags.counter)
 
 		: "cr0", "ctr", "v10", "vs0", "vs10", "vs3", "vs32", "vs33",
 		  "vs34", "fr10"
@@ -232,14 +236,14 @@ void *ping(void *input)
 	if (expecting_failure() && !is_failure(cr_)) {
 		printf("\n\tExpecting the transaction to fail, %s",
 			"but it didn't\n\t");
-		flags.result++;
+		rc = ERR_ADJUST;
 	}
 
 	/* Check if we were not expecting a failure and a it occurred. */
 	if (!expecting_failure() && is_failure(cr_)) {
 		printf("\n\tUnexpected transaction failure 0x%02lx\n\t",
 			failure_code());
-		return (void *) -1;
+		rc = ERR_RETRY;
 	}
 
 	/*
@@ -249,7 +253,7 @@ void *ping(void *input)
 	if (is_failure(cr_) && !failure_is_unavailable()) {
 		printf("\n\tUnexpected failure cause 0x%02lx\n\t",
 			failure_code());
-		return (void *) -1;
+		rc = ERR_RETRY;
 	}
 
 	/* 0x4 is a success and 0xa is a fail. See comment in is_failure(). */
@@ -276,7 +280,7 @@ void *ping(void *input)
 
 	putchar('\n');
 
-	return NULL;
+	return (void *)rc;
 }
 
 /* Thread to force context switch */
@@ -291,6 +295,55 @@ void *pong(void *not_used)
 		sched_yield();
 }
 
+static void flags_set_counter(struct Flags *flags)
+{
+	uint64_t cr_;
+	int count = 0;
+
+	do {
+		if (count == 0)
+			printf("\tTrying 0x%08" PRIx64 "... ", flags->counter);
+		else
+			printf("%d, ", count);
+		fflush(stdout);
+		asm (
+			/*
+			 * Wait an amount of context switches so
+			 * load_fp and load_vec overflow and MSR.FP,
+			 * MSR.VEC, and MSR.VSX become zero (off).
+			 */
+			"	mtctr		%[counter]		;"
+
+			/* Decrement CTR branch if CTR non zero. */
+			"1:	bdnz 1b					;"
+			"	tbegin.					;"
+			"	beq		tfail			;"
+
+			/* Get a facility unavailable */
+			"	fadd		10, 10, 10		;"
+			"	tend.                                   ;"
+			"tfail:                                         ;"
+
+			/*
+			 * Give CR back to C so that it can check what
+			 * happened.
+			 */
+			"	mfcr		%[cr_]			;"
+
+			: [cr_]       "+r" (cr_)
+			: [counter]   "r"  (flags->counter)
+			: "cr0", "ctr", "fr10"
+			);
+		count++;
+		if (!is_failure(cr_) || !failure_is_unavailable()) {
+			count = 0;
+			flags->counter += COUNTER_INCREMENT;
+			putchar('\n');
+		}
+	} while (count < 3);
+	printf("3\n");
+}
+
 /* Function that creates a thread and launches the "ping" task. */
 void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
 {
@@ -322,6 +375,17 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
 		if (rc)
 			pr_err(rc, "pthread_join");
 
+		if ((long)ret_value == ERR_ADJUST) {
+			printf("Adjusting the facility unavailable spin time...\n");
+			/*
+			 * Be a bit more agressive just now - we'd
+			 * really like to have it work
+			 */
+			flags.counter += (2 * COUNTER_INCREMENT);
+			flags_set_counter(&flags);
+			printf("Now using 0x%08" PRIx64 "\n", flags.counter);
+		}
+
 		retries--;
 	} while (ret_value != NULL && retries);
 
@@ -340,6 +404,18 @@ int main(int argc, char **argv)
 	pthread_attr_t attr;
 	cpu_set_t cpuset;
 
+	/*
+	 * Default counter for busy wait.
+	 * 0x18000000 is a good baseline determined by experimentation
+	 * on a POWER8
+	 * The autodetecting code will bump it up if it too low.
+	 */
+	flags.counter = 0x18000000;
+
+	printf("Testing required spin time required for facility unavailable...\n");
+	flags_set_counter(&flags);
+	printf("Spin time required for a reliable facility unavailable TM failure: 0x%" PRIx64 "\n",
+			flags.counter);
 	/* Set only CPU 0 in the mask. Both threads will be bound to CPU 0. */
 	CPU_ZERO(&cpuset);
 	CPU_SET(0, &cpuset);
-- 
2.15.0

Re: [PATCH 2/2] selftests/powerpc: Calculate spin time in tm-unavailable

[Gustavo Romero]

Hi Cyril,

On 21-11-2017 05:17, Cyril Bur wrote:
> Currently the tm-unavailable test spins for a fixed amount of time in
> an attempt to ensure the FPU/VMX/VSX facilities are off. This value was
> experimentally tested to be long enough.
> 
> Problems may arise if kernel heuristics were to change. This patch
> should future proof this test.

I've tried it on a VM running on '4.14.0-rc7' and apparently it gets stuck /
pretty slow on calibration, since it ran ~7m without finding the correct value
(before it would take about 3m), like:

$ time ./tm-unavailable
Testing required spin time required for facility unavailable...
	Trying 0x18000000...
	Trying 0x19000000...
	Trying 0x1a000000...
...
	Trying 0xfd000000... ^C

real	7m15.304s
user	7m15.291s
sys	0m0.004s

Trying it on a BM running on '4.13.0-rc2' it indeed found an initial value for
the timeout but for some reason the value was not sufficient for the subsequent
tests and the value raised more and more  (I understand that it's an expected
behavior tho). Even tho it runs about half the time (~3m, good!) but I think the
output could be little bit less "overloaded":

$ ./tm-unavailable
Testing required spin time required for facility unavailable...
	Trying 0x18000000...
	Trying 0x19000000...
	Trying 0x1a000000...
	Trying 0x1b000000...
	Trying 0x1c000000...
	Trying 0x1d000000...
	Trying 0x1e000000...
	Trying 0x1f000000... 1, 2, 3
Spin time required for a reliable facility unavailable TM failure: 0x1f000000
Checking if FP/VEC registers are sane after a FP unavailable exception...
If MSR.FP=0 MSR.VEC=0:
	Expecting the transaction to fail, but it didn't
	FP ok VEC ok
Adjusting the facility unavailable spin time...
	Trying 0x21000000... 1, 2, 3
Now using 0x21000000
If MSR.FP=0 MSR.VEC=0:
	Expecting the transaction to fail, but it didn't
	FP ok VEC ok
Adjusting the facility unavailable spin time...
	Trying 0x23000000... 1, 2, 3
Now using 0x23000000
If MSR.FP=1 MSR.VEC=0: FP ok VEC ok
If MSR.FP=0 MSR.VEC=1:
	Expecting the transaction to fail, but it didn't
	FP ok VEC ok
Now using 0x47000000
...

So, putting output question aside, are you getting a different result on VM,
i.e. did you notice if it got stuck/pretty slow?


Regards,
Gustavo

> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
> Because the test no longer needs to use such a conservative time for
> the busy wait, it actually runs much faster.
> 
> 
>  .../testing/selftests/powerpc/tm/tm-unavailable.c  | 92 ++++++++++++++++++++--
>  1 file changed, 84 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> index e6a0fad2bfd0..54aeb7a7fbb1 100644
> --- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> +++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> @@ -33,6 +33,11 @@
>  #define VEC_UNA_EXCEPTION	1
>  #define VSX_UNA_EXCEPTION	2
> 
> +#define ERR_RETRY 1
> +#define ERR_ADJUST 2
> +
> +#define COUNTER_INCREMENT (0x1000000)
> +
>  #define NUM_EXCEPTIONS		3
>  #define err_at_line(status, errnum, format, ...) \
>  	error_at_line(status, errnum,  __FILE__, __LINE__, format ##__VA_ARGS__)
> @@ -45,6 +50,7 @@ struct Flags {
>  	int touch_vec;
>  	int result;
>  	int exception;
> +	uint64_t counter;
>  } flags;
> 
>  bool expecting_failure(void)
> @@ -87,14 +93,12 @@ void *ping(void *input)
>  	 * Expected values for vs0 and vs32 after a TM failure. They must never
>  	 * change, otherwise they got corrupted.
>  	 */
> +	long rc = 0;
>  	uint64_t high_vs0 = 0x5555555555555555;
>  	uint64_t low_vs0 = 0xffffffffffffffff;
>  	uint64_t high_vs32 = 0x5555555555555555;
>  	uint64_t low_vs32 = 0xffffffffffffffff;
> 
> -	/* Counter for busy wait */
> -	uint64_t counter = 0x1ff000000;
> -
>  	/*
>  	 * Variable to keep a copy of CR register content taken just after we
>  	 * leave the transactional state.
> @@ -217,7 +221,7 @@ void *ping(void *input)
>  		  [ex_fp]     "i"  (FP_UNA_EXCEPTION),
>  		  [ex_vec]    "i"  (VEC_UNA_EXCEPTION),
>  		  [ex_vsx]    "i"  (VSX_UNA_EXCEPTION),
> -		  [counter]   "r"  (counter)
> +		  [counter]   "r"  (flags.counter)
> 
>  		: "cr0", "ctr", "v10", "vs0", "vs10", "vs3", "vs32", "vs33",
>  		  "vs34", "fr10"
> @@ -232,14 +236,14 @@ void *ping(void *input)
>  	if (expecting_failure() && !is_failure(cr_)) {
>  		printf("\n\tExpecting the transaction to fail, %s",
>  			"but it didn't\n\t");
> -		flags.result++;
> +		rc = ERR_ADJUST;
>  	}
> 
>  	/* Check if we were not expecting a failure and a it occurred. */
>  	if (!expecting_failure() && is_failure(cr_)) {
>  		printf("\n\tUnexpected transaction failure 0x%02lx\n\t",
>  			failure_code());
> -		return (void *) -1;
> +		rc = ERR_RETRY;
>  	}
> 
>  	/*
> @@ -249,7 +253,7 @@ void *ping(void *input)
>  	if (is_failure(cr_) && !failure_is_unavailable()) {
>  		printf("\n\tUnexpected failure cause 0x%02lx\n\t",
>  			failure_code());
> -		return (void *) -1;
> +		rc = ERR_RETRY;
>  	}
> 
>  	/* 0x4 is a success and 0xa is a fail. See comment in is_failure(). */
> @@ -276,7 +280,7 @@ void *ping(void *input)
> 
>  	putchar('\n');
> 
> -	return NULL;
> +	return (void *)rc;
>  }
> 
>  /* Thread to force context switch */
> @@ -291,6 +295,55 @@ void *pong(void *not_used)
>  		sched_yield();
>  }
> 
> +static void flags_set_counter(struct Flags *flags)
> +{
> +	uint64_t cr_;
> +	int count = 0;
> +
> +	do {
> +		if (count == 0)
> +			printf("\tTrying 0x%08" PRIx64 "... ", flags->counter);
> +		else
> +			printf("%d, ", count);
> +		fflush(stdout);
> +		asm (
> +			/*
> +			 * Wait an amount of context switches so
> +			 * load_fp and load_vec overflow and MSR.FP,
> +			 * MSR.VEC, and MSR.VSX become zero (off).
> +			 */
> +			"	mtctr		%[counter]		;"
> +
> +			/* Decrement CTR branch if CTR non zero. */
> +			"1:	bdnz 1b					;"
> +			"	tbegin.					;"
> +			"	beq		tfail			;"
> +
> +			/* Get a facility unavailable */
> +			"	fadd		10, 10, 10		;"
> +			"	tend.                                   ;"
> +			"tfail:                                         ;"
> +
> +			/*
> +			 * Give CR back to C so that it can check what
> +			 * happened.
> +			 */
> +			"	mfcr		%[cr_]			;"
> +
> +			: [cr_]       "+r" (cr_)
> +			: [counter]   "r"  (flags->counter)
> +			: "cr0", "ctr", "fr10"
> +			);
> +		count++;
> +		if (!is_failure(cr_) || !failure_is_unavailable()) {
> +			count = 0;
> +			flags->counter += COUNTER_INCREMENT;
> +			putchar('\n');
> +		}
> +	} while (count < 3);
> +	printf("3\n");
> +}
> +
>  /* Function that creates a thread and launches the "ping" task. */
>  void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
>  {
> @@ -322,6 +375,17 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
>  		if (rc)
>  			pr_err(rc, "pthread_join");
> 
> +		if ((long)ret_value == ERR_ADJUST) {
> +			printf("Adjusting the facility unavailable spin time...\n");
> +			/*
> +			 * Be a bit more agressive just now - we'd
> +			 * really like to have it work
> +			 */
> +			flags.counter += (2 * COUNTER_INCREMENT);
> +			flags_set_counter(&flags);
> +			printf("Now using 0x%08" PRIx64 "\n", flags.counter);
> +		}
> +
>  		retries--;
>  	} while (ret_value != NULL && retries);
> 
> @@ -340,6 +404,18 @@ int main(int argc, char **argv)
>  	pthread_attr_t attr;
>  	cpu_set_t cpuset;
> 
> +	/*
> +	 * Default counter for busy wait.
> +	 * 0x18000000 is a good baseline determined by experimentation
> +	 * on a POWER8
> +	 * The autodetecting code will bump it up if it too low.
> +	 */
> +	flags.counter = 0x18000000;
> +
> +	printf("Testing required spin time required for facility unavailable...\n");
> +	flags_set_counter(&flags);
> +	printf("Spin time required for a reliable facility unavailable TM failure: 0x%" PRIx64 "\n",
> +			flags.counter);
>  	/* Set only CPU 0 in the mask. Both threads will be bound to CPU 0. */
>  	CPU_ZERO(&cpuset);
>  	CPU_SET(0, &cpuset);
> 

Re: [PATCH 2/2] selftests/powerpc: Calculate spin time in tm-unavailable

[Cyril Bur]

On Tue, 2017-11-21 at 11:31 -0200, Gustavo Romero wrote:
> Hi Cyril,
> 
> On 21-11-2017 05:17, Cyril Bur wrote:
> > Currently the tm-unavailable test spins for a fixed amount of time in
> > an attempt to ensure the FPU/VMX/VSX facilities are off. This value was
> > experimentally tested to be long enough.
> > 
> > Problems may arise if kernel heuristics were to change. This patch
> > should future proof this test.
> 
> I've tried it on a VM running on '4.14.0-rc7' and apparently it gets stuck
> pretty slow on calibration, since it ran ~7m without finding the correct value
> (before it would take about 3m), like:
> 
> $ time ./tm-unavailable
> Testing required spin time required for facility unavailable...
> 	Trying 0x18000000...
> 	Trying 0x19000000...
> 	Trying 0x1a000000...
> ...
> 	Trying 0xfd000000... ^C
> 
> real	7m15.304s
> user	7m15.291s
> sys	0m0.004s
> 

Interesting! I didn't test in a VM. I guess hypervisor switching
completely changes the heuristic. Ok I'll have to rethink.

Maybe the increase should be a multiplier to get to a good state more
quickly.

> Trying it on a BM running on '4.13.0-rc2' it indeed found an initial value for
> the timeout but for some reason the value was not sufficient for the subsequent
> tests and the value raised more and more  (I understand that it's an expected
> behavior tho). Even tho it runs about half the time (~3m, good!) but I think the
> output could be little bit less "overloaded":
> 

Happy to put some (or all) of that output inside if (DEBUG)

> $ ./tm-unavailable
> Testing required spin time required for facility unavailable...
> 	Trying 0x18000000...
> 	Trying 0x19000000...
> 	Trying 0x1a000000...
> 	Trying 0x1b000000...
> 	Trying 0x1c000000...
> 	Trying 0x1d000000...
> 	Trying 0x1e000000...
> 	Trying 0x1f000000... 1, 2, 3
> Spin time required for a reliable facility unavailable TM failure: 0x1f000000
> Checking if FP/VEC registers are sane after a FP unavailable exception...
> If MSR.FP=0 MSR.VEC=0:
> 	Expecting the transaction to fail, but it didn't
> 	FP ok VEC ok
> Adjusting the facility unavailable spin time...
> 	Trying 0x21000000... 1, 2, 3
> Now using 0x21000000
> If MSR.FP=0 MSR.VEC=0:
> 	Expecting the transaction to fail, but it didn't
> 	FP ok VEC ok
> Adjusting the facility unavailable spin time...
> 	Trying 0x23000000... 1, 2, 3
> Now using 0x23000000
> If MSR.FP=1 MSR.VEC=0: FP ok VEC ok
> If MSR.FP=0 MSR.VEC=1:
> 	Expecting the transaction to fail, but it didn't
> 	FP ok VEC ok
> Now using 0x47000000
> ...
> 
> So, putting output question aside, are you getting a different result on VM,
> i.e. did you notice if it got stuck/pretty slow?
> 
> 
> Regards,
> Gustavo
> 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> > Because the test no longer needs to use such a conservative time for
> > the busy wait, it actually runs much faster.
> > 
> > 
> >  .../testing/selftests/powerpc/tm/tm-unavailable.c  | 92 ++++++++++++++++++++--
> >  1 file changed, 84 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> > index e6a0fad2bfd0..54aeb7a7fbb1 100644
> > --- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> > +++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> > @@ -33,6 +33,11 @@
> >  #define VEC_UNA_EXCEPTION	1
> >  #define VSX_UNA_EXCEPTION	2
> > 
> > +#define ERR_RETRY 1
> > +#define ERR_ADJUST 2
> > +
> > +#define COUNTER_INCREMENT (0x1000000)
> > +
> >  #define NUM_EXCEPTIONS		3
> >  #define err_at_line(status, errnum, format, ...) \
> >  	error_at_line(status, errnum,  __FILE__, __LINE__, format ##__VA_ARGS__)
> > @@ -45,6 +50,7 @@ struct Flags {
> >  	int touch_vec;
> >  	int result;
> >  	int exception;
> > +	uint64_t counter;
> >  } flags;
> > 
> >  bool expecting_failure(void)
> > @@ -87,14 +93,12 @@ void *ping(void *input)
> >  	 * Expected values for vs0 and vs32 after a TM failure. They must never
> >  	 * change, otherwise they got corrupted.
> >  	 */
> > +	long rc = 0;
> >  	uint64_t high_vs0 = 0x5555555555555555;
> >  	uint64_t low_vs0 = 0xffffffffffffffff;
> >  	uint64_t high_vs32 = 0x5555555555555555;
> >  	uint64_t low_vs32 = 0xffffffffffffffff;
> > 
> > -	/* Counter for busy wait */
> > -	uint64_t counter = 0x1ff000000;
> > -
> >  	/*
> >  	 * Variable to keep a copy of CR register content taken just after we
> >  	 * leave the transactional state.
> > @@ -217,7 +221,7 @@ void *ping(void *input)
> >  		  [ex_fp]     "i"  (FP_UNA_EXCEPTION),
> >  		  [ex_vec]    "i"  (VEC_UNA_EXCEPTION),
> >  		  [ex_vsx]    "i"  (VSX_UNA_EXCEPTION),
> > -		  [counter]   "r"  (counter)
> > +		  [counter]   "r"  (flags.counter)
> > 
> >  		: "cr0", "ctr", "v10", "vs0", "vs10", "vs3", "vs32", "vs33",
> >  		  "vs34", "fr10"
> > @@ -232,14 +236,14 @@ void *ping(void *input)
> >  	if (expecting_failure() && !is_failure(cr_)) {
> >  		printf("\n\tExpecting the transaction to fail, %s",
> >  			"but it didn't\n\t");
> > -		flags.result++;
> > +		rc = ERR_ADJUST;
> >  	}
> > 
> >  	/* Check if we were not expecting a failure and a it occurred. */
> >  	if (!expecting_failure() && is_failure(cr_)) {
> >  		printf("\n\tUnexpected transaction failure 0x%02lx\n\t",
> >  			failure_code());
> > -		return (void *) -1;
> > +		rc = ERR_RETRY;
> >  	}
> > 
> >  	/*
> > @@ -249,7 +253,7 @@ void *ping(void *input)
> >  	if (is_failure(cr_) && !failure_is_unavailable()) {
> >  		printf("\n\tUnexpected failure cause 0x%02lx\n\t",
> >  			failure_code());
> > -		return (void *) -1;
> > +		rc = ERR_RETRY;
> >  	}
> > 
> >  	/* 0x4 is a success and 0xa is a fail. See comment in is_failure(). */
> > @@ -276,7 +280,7 @@ void *ping(void *input)
> > 
> >  	putchar('\n');
> > 
> > -	return NULL;
> > +	return (void *)rc;
> >  }
> > 
> >  /* Thread to force context switch */
> > @@ -291,6 +295,55 @@ void *pong(void *not_used)
> >  		sched_yield();
> >  }
> > 
> > +static void flags_set_counter(struct Flags *flags)
> > +{
> > +	uint64_t cr_;
> > +	int count = 0;
> > +
> > +	do {
> > +		if (count == 0)
> > +			printf("\tTrying 0x%08" PRIx64 "... ", flags->counter);
> > +		else
> > +			printf("%d, ", count);
> > +		fflush(stdout);
> > +		asm (
> > +			/*
> > +			 * Wait an amount of context switches so
> > +			 * load_fp and load_vec overflow and MSR.FP,
> > +			 * MSR.VEC, and MSR.VSX become zero (off).
> > +			 */
> > +			"	mtctr		%[counter]		;"
> > +
> > +			/* Decrement CTR branch if CTR non zero. */
> > +			"1:	bdnz 1b					;"
> > +			"	tbegin.					;"
> > +			"	beq		tfail			;"
> > +
> > +			/* Get a facility unavailable */
> > +			"	fadd		10, 10, 10		;"
> > +			"	tend.                                   ;"
> > +			"tfail:                                         ;"
> > +
> > +			/*
> > +			 * Give CR back to C so that it can check what
> > +			 * happened.
> > +			 */
> > +			"	mfcr		%[cr_]			;"
> > +
> > +			: [cr_]       "+r" (cr_)
> > +			: [counter]   "r"  (flags->counter)
> > +			: "cr0", "ctr", "fr10"
> > +			);
> > +		count++;
> > +		if (!is_failure(cr_) || !failure_is_unavailable()) {
> > +			count = 0;
> > +			flags->counter += COUNTER_INCREMENT;
> > +			putchar('\n');
> > +		}
> > +	} while (count < 3);
> > +	printf("3\n");
> > +}
> > +
> >  /* Function that creates a thread and launches the "ping" task. */
> >  void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
> >  {
> > @@ -322,6 +375,17 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
> >  		if (rc)
> >  			pr_err(rc, "pthread_join");
> > 
> > +		if ((long)ret_value == ERR_ADJUST) {
> > +			printf("Adjusting the facility unavailable spin time...\n");
> > +			/*
> > +			 * Be a bit more agressive just now - we'd
> > +			 * really like to have it work
> > +			 */
> > +			flags.counter += (2 * COUNTER_INCREMENT);
> > +			flags_set_counter(&flags);
> > +			printf("Now using 0x%08" PRIx64 "\n", flags.counter);
> > +		}
> > +
> >  		retries--;
> >  	} while (ret_value != NULL && retries);
> > 
> > @@ -340,6 +404,18 @@ int main(int argc, char **argv)
> >  	pthread_attr_t attr;
> >  	cpu_set_t cpuset;
> > 
> > +	/*
> > +	 * Default counter for busy wait.
> > +	 * 0x18000000 is a good baseline determined by experimentation
> > +	 * on a POWER8
> > +	 * The autodetecting code will bump it up if it too low.
> > +	 */
> > +	flags.counter = 0x18000000;
> > +
> > +	printf("Testing required spin time required for facility unavailable...\n");
> > +	flags_set_counter(&flags);
> > +	printf("Spin time required for a reliable facility unavailable TM failure: 0x%" PRIx64 "\n",
> > +			flags.counter);
> >  	/* Set only CPU 0 in the mask. Both threads will be bound to CPU 0. */
> >  	CPU_ZERO(&cpuset);
> >  	CPU_SET(0, &cpuset);
> > 
> 
> 

Re: [PATCH 2/2] selftests/powerpc: Calculate spin time in tm-unavailable

[Michael Ellerman]

Cyril Bur <cyrilbur@gmail.com> writes:

> On Tue, 2017-11-21 at 11:31 -0200, Gustavo Romero wrote:
>> Hi Cyril,
>> 
>> On 21-11-2017 05:17, Cyril Bur wrote:
>> > Currently the tm-unavailable test spins for a fixed amount of time in
>> > an attempt to ensure the FPU/VMX/VSX facilities are off. This value was
>> > experimentally tested to be long enough.
>> > 
>> > Problems may arise if kernel heuristics were to change. This patch
>> > should future proof this test.
>> 
>> I've tried it on a VM running on '4.14.0-rc7' and apparently it gets stuck
>> pretty slow on calibration, since it ran ~7m without finding the correct value
>> (before it would take about 3m), like:
>> 
>> $ time ./tm-unavailable
>> Testing required spin time required for facility unavailable...
>> 	Trying 0x18000000...
>> 	Trying 0x19000000...
>> 	Trying 0x1a000000...
>> ...
>> 	Trying 0xfd000000... ^C
>> 
>> real	7m15.304s
>> user	7m15.291s
>> sys	0m0.004s
>> 
>
> Interesting! I didn't test in a VM. I guess hypervisor switching
> completely changes the heuristic. Ok I'll have to rethink.
>
> Maybe the increase should be a multiplier to get to a good state more
> quickly.

Yeah this sucks in a VM:

# /home/michael/tm-unavailable 
Testing required spin time required for facility unavailable...
	Trying 0x18000000... 
	Trying 0x19000000... 
...
	Trying 0x110000000...  

etc.

I got sick of waiting for it, but it's causing my selftests job to time
out so it must be taking > ~1 hour.

cheers

Re: [PATCH 2/2] selftests/powerpc: Calculate spin time in tm-unavailable

[Cyril Bur]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

On Mon, 2017-12-11 at 13:02 +1100, Michael Ellerman wrote:

> Cyril Bur <cyrilbur@gmail.com> writes:

>

> > On Tue, 2017-11-21 at 11:31 -0200, Gustavo Romero wrote:

> > > Hi Cyril,

> > >

> > > On 21-11-2017 05:17, Cyril Bur wrote:

> > > > Currently the tm-unavailable test spins for a fixed amount of time in

> > > > an attempt to ensure the FPU/VMX/VSX facilities are off. This value was

> > > > experimentally tested to be long enough.

> > > >

> > > > Problems may arise if kernel heuristics were to change. This patch

> > > > should future proof this test.

> > >

> > > I've tried it on a VM running on '4.14.0-rc7' and apparently it gets stuck

> > > pretty slow on calibration, since it ran ~7m without finding the correct value

> > > (before it would take about 3m), like:

> > >

> > > $ time ./tm-unavailable

> > > Testing required spin time required for facility unavailable...

> > > 	Trying 0x18000000...

> > > 	Trying 0x19000000...

> > > 	Trying 0x1a000000...

> > > ...

> > > 	Trying 0xfd000000... ^C

> > >

> > > real	7m15.304s

> > > user	7m15.291s

> > > sys	0m0.004s

> > >

> >

> > Interesting! I didn't test in a VM. I guess hypervisor switching

> > completely changes the heuristic. Ok I'll have to rethink.

> >

> > Maybe the increase should be a multiplier to get to a good state more

> > quickly.

>

> Yeah this sucks in a VM:

>

> # /home/michael/tm-unavailable

> Testing required spin time required for facility unavailable...

> 	Trying 0x18000000...

> 	Trying 0x19000000...

> ...

> 	Trying 0x110000000...

>

> etc.

>

> I got sick of waiting for it, but it's causing my selftests job to time

> out so it must be taking > ~1 hour.

>


Yeah sorry, I'll see if I can come up with a better way for a VM. Needs a
few more cycles from me.

Cyril

> cheers

[-- Attachment #2: Type: text/html, Size: 14857 bytes --]

Re: [PATCH 1/2] selftests/powerpc: Check for pthread errors in tm-unavailable

[Gustavo Romero]

Hi Cyril,

Thanks for adding the checks!

On 21-11-2017 05:17, Cyril Bur wrote:
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>

Signed-off-by: Gustavo Romero <gromero@linux.vnet.ibm.com>

Cheers,
Gustavo

> ---
>  .../testing/selftests/powerpc/tm/tm-unavailable.c  | 43 +++++++++++++++++-----
>  1 file changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> index 96c37f84ce54..e6a0fad2bfd0 100644
> --- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> +++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> @@ -15,6 +15,7 @@
>   */
> 
>  #define _GNU_SOURCE
> +#include <error.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <unistd.h>
> @@ -33,6 +34,11 @@
>  #define VSX_UNA_EXCEPTION	2
> 
>  #define NUM_EXCEPTIONS		3
> +#define err_at_line(status, errnum, format, ...) \
> +	error_at_line(status, errnum,  __FILE__, __LINE__, format ##__VA_ARGS__)
> +
> +#define pr_warn(code, format, ...) err_at_line(0, code, format, ##__VA_ARGS__)
> +#define pr_err(code, format, ...) err_at_line(1, code, format, ##__VA_ARGS__)
> 
>  struct Flags {
>  	int touch_fp;
> @@ -303,10 +309,19 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
>  	 * checking if the failure cause is the one we expect.
>  	 */
>  	do {
> +		int rc;
> +
>  		/* Bind 'ping' to CPU 0, as specified in 'attr'. */
> -		pthread_create(&t0, attr, ping, (void *) &flags);
> -		pthread_setname_np(t0, "ping");
> -		pthread_join(t0, &ret_value);
> +		rc = pthread_create(&t0, attr, ping, (void *) &flags);
> +		if (rc)
> +			pr_err(rc, "pthread_create()");
> +		rc = pthread_setname_np(t0, "ping");
> +		if (rc)
> +			pr_warn(rc, "pthread_setname_np");
> +		rc = pthread_join(t0, &ret_value);
> +		if (rc)
> +			pr_err(rc, "pthread_join");
> +
>  		retries--;
>  	} while (ret_value != NULL && retries);
> 
> @@ -320,7 +335,7 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
> 
>  int main(int argc, char **argv)
>  {
> -	int exception; /* FP = 0, VEC = 1, VSX = 2 */
> +	int rc, exception; /* FP = 0, VEC = 1, VSX = 2 */
>  	pthread_t t1;
>  	pthread_attr_t attr;
>  	cpu_set_t cpuset;
> @@ -330,13 +345,23 @@ int main(int argc, char **argv)
>  	CPU_SET(0, &cpuset);
> 
>  	/* Init pthread attribute. */
> -	pthread_attr_init(&attr);
> +	rc = pthread_attr_init(&attr);
> +	if (rc)
> +		pr_err(rc, "pthread_attr_init()");
> 
>  	/* Set CPU 0 mask into the pthread attribute. */
> -	pthread_attr_setaffinity_np(&attr, sizeof(cpu_set_t), &cpuset);
> -
> -	pthread_create(&t1, &attr /* Bind 'pong' to CPU 0 */, pong, NULL);
> -	pthread_setname_np(t1, "pong"); /* Name it for systemtap convenience */
> +	rc = pthread_attr_setaffinity_np(&attr, sizeof(cpu_set_t), &cpuset);
> +	if (rc)
> +		pr_err(rc, "pthread_attr_setaffinity_np()");
> +
> +	rc = pthread_create(&t1, &attr /* Bind 'pong' to CPU 0 */, pong, NULL);
> +	if (rc)
> +		pr_err(rc, "pthread_create()");
> +
> +	/* Name it for systemtap convenience */
> +	rc = pthread_setname_np(t1, "pong");
> +	if (rc)
> +		pr_warn(rc, "pthread_create()");
> 
>  	flags.result = 0;
> 

Re: [1/2] selftests/powerpc: Check for pthread errors in tm-unavailable

[Michael Ellerman]

On Tue, 2017-11-21 at 07:17:19 UTC, Cyril Bur wrote:
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> Signed-off-by: Gustavo Romero <gromero@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/5783ee6ec3d3323a04cc69764d57ca

cheers