[PATCH 0/3] rt-tests: pi_stress fixes

[PATCH 0/3] rt-tests: pi_stress fixes

[John Kacur]

I have tested these patches on a Nehalem machine with 4 cores and HT.
It would be good if folks with larger machines tested too.

I pushed the changes to
git://git.kernel.org/pub/scm/linux/kernel/git/jkacur/rt-tests.git
branch rt-tests-dev
In the order they should be applied starting with the first:
97dbb3cebd200a4d45d89d8e264e43b119b2cbcf 
9093a5de51964f3a9ab1454a77181fad2d3e7217
2712903a40560378cb61336994258780cfbb83d9

Thanks


John Kacur (3):
  rt-tests: pi_stress: Use a pthread_mutex_t for the global variable
    shutdown
  rt-tests: pi_stress: Check whether quiet is set, before taking
    shutdown_mtx
  rt-tests: pi_stress: Remove racy state variables that cause watchdog
    to trigger

 src/pi_tests/pi_stress.c |   58 +++++++++++++++++++++++++++-------------------
 1 files changed, 34 insertions(+), 24 deletions(-)

[PATCH 1/3] rt-tests: pi_stress: Use a pthread_mutex_t for the global variable shutdown

[John Kacur]

- Use a pthread_mutex_t for the global variable shutdown.
- Remove the volatile qualifier from shutdown. (Since the original author
probably simply meant the variable should be atomic which we effectively
get through the mutex.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 src/pi_tests/pi_stress.c |   25 ++++++++++++++++++++++---
 1 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/src/pi_tests/pi_stress.c b/src/pi_tests/pi_stress.c
index 76f466d..44c8f69 100644
--- a/src/pi_tests/pi_stress.c
+++ b/src/pi_tests/pi_stress.c
@@ -118,8 +118,8 @@ int prompt = 0;
 /* report interval */
 unsigned long report_interval = (unsigned long)SEC_TO_USEC(0.75);
 
-/* global that indicates we should shut down */
-volatile int shutdown = 0;
+int shutdown = 0;		/* global indicating we should shut down */
+pthread_mutex_t shutdown_mtx;	/* associated mutex */
 
 /* indicate if errors have occured */
 int have_errors = 0;
@@ -550,14 +550,23 @@ void *reporter(void *arg)
 	debug("reporter: starting report loop\n");
 	info("Press Control-C to stop test\nCurrent Inversions: \n");
 
-	while (shutdown == 0) {
+	for (;;) {
+		pthread_mutex_lock(&shutdown_mtx);
+		if (shutdown) {
+			pthread_mutex_unlock(&shutdown_mtx);
+			break;
+		}
+		pthread_mutex_unlock(&shutdown_mtx);
+
 		/* wait for our reporting interval */
 		status = clock_nanosleep(CLOCK_MONOTONIC, 0, &ts, NULL);
 		if (status) {
 			error("from clock_nanosleep: %s\n", strerror(status));
 			break;
 		}
+
 		/* check for signaled shutdown */
+		pthread_mutex_lock(&shutdown_mtx);
 		if (shutdown == 0) {
 			if (!quiet) {
 				fputs(UP_ONE, stdout);
@@ -565,6 +574,8 @@ void *reporter(void *arg)
 				       total_inversions());
 			}
 		}
+		pthread_mutex_unlock(&shutdown_mtx);
+
 		/* if we specified a duration, see if it has expired */
 		if (end && time(NULL) > end) {
 			info("duration reached (%d seconds)\n", duration);
@@ -660,11 +671,13 @@ void *low_priority(void *arg)
 
 		/* Only one Thread needs to check the shutdown status */
 		if (status == PTHREAD_BARRIER_SERIAL_THREAD) {
+			pthread_mutex_lock(&shutdown_mtx);
 			if (shutdown) {
 				pthread_mutex_lock(loop_mtx);
 				*loop = 0;
 				pthread_mutex_unlock(loop_mtx);
 			}
+			pthread_mutex_unlock(&shutdown_mtx);
 		}
 
 		/* initial state */
@@ -781,11 +794,13 @@ void *med_priority(void *arg)
 
 		/* Only one Thread needs to check the shutdown status */
 		if (status == PTHREAD_BARRIER_SERIAL_THREAD) {
+			pthread_mutex_lock(&shutdown_mtx);
 			if (shutdown) {
 				pthread_mutex_lock(loop_mtx);
 				*loop = 0;
 				pthread_mutex_unlock(loop_mtx);
 			}
+			pthread_mutex_unlock(&shutdown_mtx);
 		}
 
 		/* start state */
@@ -885,11 +900,13 @@ void *high_priority(void *arg)
 
 		/* Only one Thread needs to check the shutdown status */
 		if (status == PTHREAD_BARRIER_SERIAL_THREAD) {
+			pthread_mutex_lock(&shutdown_mtx);
 			if (shutdown) {
 				pthread_mutex_lock(loop_mtx);
 				*loop = 0;
 				pthread_mutex_unlock(loop_mtx);
 			}
+			pthread_mutex_unlock(&shutdown_mtx);
 		}
 		p->high_has_run = 0;
 		debug("high_priority[%d]: entering start state (%d)\n", p->id,
@@ -1051,11 +1068,13 @@ int allow_sigterm(void)
 /* clean up before exiting */
 void set_shutdown_flag(void)
 {
+	pthread_mutex_lock(&shutdown_mtx);
 	if (shutdown == 0) {
 		/* tell anyone that's looking that we're done */
 		info("setting shutdown flag\n");
 		shutdown = 1;
 	}
+	pthread_mutex_unlock(&shutdown_mtx);
 }
 
 /* set up a test group */
-- 
1.6.2.5

[PATCH 2/3] rt-tests: pi_stress: Check whether quiet is set, before taking shutdown_mtx

[John Kacur]

- Check whether quiet is set, before taking shutdown_mtx
- Add quiet to the help menu.
- Remove unused "signal" from struct options

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 src/pi_tests/pi_stress.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/pi_tests/pi_stress.c b/src/pi_tests/pi_stress.c
index 44c8f69..f4b43ed 100644
--- a/src/pi_tests/pi_stress.c
+++ b/src/pi_tests/pi_stress.c
@@ -109,8 +109,7 @@ int verbose = 0;
 /* turn on debugging prints */
 int debugging = 0;
 
-/* turn off all prints */
-int quiet = 0;
+int quiet = 0;	/* turn off all prints, default = 0 (off) */
 
 /* prompt to start test */
 int prompt = 0;
@@ -141,7 +140,6 @@ struct option options[] = {
 	{"groups", required_argument, NULL, 'g'},
 	{"inversions", required_argument, NULL, 'i'},
 	{"rr", no_argument, NULL, 'r'},
-	{"signal", no_argument, NULL, 's'},
 	{"uniprocessor", no_argument, NULL, 'u'},
 	{"prompt", no_argument, NULL, 'p'},
 	{"debug", no_argument, NULL, 'd'},
@@ -566,12 +564,12 @@ void *reporter(void *arg)
 		}
 
 		/* check for signaled shutdown */
-		pthread_mutex_lock(&shutdown_mtx);
-		if (shutdown == 0) {
-			if (!quiet) {
+		if (!quiet) {
+			pthread_mutex_lock(&shutdown_mtx);
+			if (shutdown == 0) {
 				fputs(UP_ONE, stdout);
 				printf("Current Inversions: %lu\n",
-				       total_inversions());
+						total_inversions());
 			}
 		}
 		pthread_mutex_unlock(&shutdown_mtx);
@@ -1004,6 +1002,7 @@ void usage(void)
 	printf("usage: pi_stress <options>\n");
 	printf("    options:\n");
 	printf("\t--verbose\t- lots of output\n");
+	printf("\t--quiet\t\t- surpress running output\n");
 	printf
 	    ("\t--duration=<n>- length of the test run in seconds [infinite]\n");
 	printf("\t--groups=<n>\t- set the number of inversion groups [%d]\n",
-- 
1.6.2.5

[PATCH 3/3] rt-tests: pi_stress: Remove racy state variables that cause watchdog to trigger

[John Kacur]

When using pthread_barrier_wait, it is important that barriers are called
the correct number of times. That is - the same number given as the count
when initializing the barrier.

There was a do-while loop around elevate_barrier in the med priority thread.
On most machines, it actually never looped.

On threads with enough processors (nehalem for example), there was a racy
situation in which the high priority thread could come out of the finish
barrier, and before it could set high_has_run = 0, the medium priority
thread would test the value and call the elevate barrier an extra time.

This patch removes the bogus loop and related state variables and fixes
the hang.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 src/pi_tests/pi_stress.c |   22 +++++++---------------
 1 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/src/pi_tests/pi_stress.c b/src/pi_tests/pi_stress.c
index f4b43ed..6f1a309 100644
--- a/src/pi_tests/pi_stress.c
+++ b/src/pi_tests/pi_stress.c
@@ -201,8 +201,6 @@ struct group_parameters {
 	int loop;	/* boolean, loop or not, connected to shutdown */
 
 	/* state variables */
-	volatile int high_has_run;
-	volatile int low_unlocked;
 	volatile int watchdog;
 
 	/* total number of inversions performed */
@@ -703,7 +701,6 @@ void *low_priority(void *arg)
 		}
 		/* wait for priority boost */
 		debug("low_priority[%d]: entering elevated wait\n", p->id);
-		p->low_unlocked = 0;	/* prevent race with med_priority */
 		status = pthread_barrier_wait(&p->elevate_barrier);
 		if (status && status != PTHREAD_BARRIER_SERIAL_THREAD) {
 			error
@@ -711,7 +708,6 @@ void *low_priority(void *arg)
 			     p->id, status);
 			return NULL;
 		}
-		p->low_unlocked = 1;
 
 		/* release the mutex */
 		debug("low_priority[%d]: unlocking mutex\n", p->id);
@@ -812,15 +808,12 @@ void *med_priority(void *arg)
 			return NULL;
 		}
 		debug("med_priority[%d]: entering elevate state\n", p->id);
-		do {
-			status = pthread_barrier_wait(&p->elevate_barrier);
-			if (status && status != PTHREAD_BARRIER_SERIAL_THREAD) {
-				error
-				    ("med_priority[%d]: pthread_barrier_wait(elevate): %x",
-				     p->id, status);
-				return NULL;
-			}
-		} while (!p->high_has_run && !p->low_unlocked);
+		status = pthread_barrier_wait(&p->elevate_barrier);
+		if (status && status != PTHREAD_BARRIER_SERIAL_THREAD) {
+			error ("med_priority[%d]: pthread_barrier_wait(elevate): %x", p->id, status);
+			return NULL;
+		}
+
 		debug("med_priority[%d]: entering finish state\n", p->id);
 		status = pthread_barrier_wait(&p->finish_barrier);
 		if (status && status != PTHREAD_BARRIER_SERIAL_THREAD) {
@@ -906,7 +899,6 @@ void *high_priority(void *arg)
 			}
 			pthread_mutex_unlock(&shutdown_mtx);
 		}
-		p->high_has_run = 0;
 		debug("high_priority[%d]: entering start state (%d)\n", p->id,
 		      count++);
 		status = pthread_barrier_wait(&p->start_barrier);
@@ -928,7 +920,7 @@ void *high_priority(void *arg)
 		debug("high_priority[%d]: locking mutex\n", p->id);
 		pthread_mutex_lock(&p->mutex);
 		debug("high_priority[%d]: got mutex\n", p->id);
-		p->high_has_run = 1;
+
 		debug("high_priority[%d]: unlocking mutex\n", p->id);
 		pthread_mutex_unlock(&p->mutex);
 		debug("high_priority[%d]: entering finish state\n", p->id);
-- 
1.6.2.5