[PATCH 1/4] cr_tests: futex: pi: Adjust RT priority range

[PATCH 1/4] cr_tests: futex: pi: Adjust RT priority range

[Matt Helsley]

Merely checking the priority values does not work on some systems. For example,
my Fedora 12 system shows default realtime priority limits of 0 (soft) 0 (hard)
even for root.

Instead, read the limits and attempt to set them such that they will work
for the test. If we fail to set them then fail the test as well.

Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 futex/pi.c |   91 ++++++++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 62 insertions(+), 29 deletions(-)

diff --git a/futex/pi.c b/futex/pi.c
index 36a357c..41f539b 100644
--- a/futex/pi.c
+++ b/futex/pi.c
@@ -552,6 +552,67 @@ out:
 	return retval;
 }
 
+void set_rtprio_range(void)
+{
+	struct rlimit lim;
+	int num_tries;
+
+	/*
+	 * These are the maximums allowed by the scheduler --
+	 * not the maximum prios we are permitted to set. Hence
+	 * the rlimit bits below.
+	 */
+	prio_min = sched_get_priority_min(sched_policy);
+	prio_max = sched_get_priority_max(sched_policy);
+	if (prio_min < 0  || prio_max < 0) {
+		log_error("sched_get_priority_min|max");
+		fclose(logfp);
+		exit(1);
+	}
+
+	if ((prio_max - prio_min) < 1) {
+		log("FAIL", "Too fewer priority levels for this test.\n");
+		fclose(logfp);
+		exit(13);
+	}
+	if (N > (prio_max - prio_min)) {
+		N = prio_max - prio_min;
+		log("WARN", "Fewer priority levels than specified processes. Using %d processes (the number of priority levels)\n", N);
+	}
+
+	for (num_tries = 2; num_tries; num_tries--) {
+
+		if (getrlimit(RLIMIT_RTPRIO, &lim) == -1) {
+			log_error("getrlimit");
+			fclose(logfp);
+			exit(12);
+		}
+		if (lim.rlim_cur == RLIM_INFINITY)
+			break;
+		if (lim.rlim_cur >= N)
+			break;
+
+		/* Else we must try to adjust the allowable range */
+		if (lim.rlim_cur < N)
+			lim.rlim_cur = N;
+		if (lim.rlim_cur > lim.rlim_max)
+			lim.rlim_max = lim.rlim_cur;
+		if (setrlimit(RLIMIT_RTPRIO, &lim) == -1) {
+			log_error("setrlimit");
+			fclose(logfp);
+			exit(10);
+		}
+	}
+
+	if (getrlimit(RLIMIT_RTPRIO, &lim) == -1) {
+		log_error("getrlimit");
+		fclose(logfp);
+		exit(13);
+	}
+	log("INFO", "RLIMIT_RTPRIO: soft (cur): %ld hard (max): %ld\n",
+		lim.rlim_cur, lim.rlim_max);
+}
+
 int main(int argc, char **argv)
 {
 	struct sched_param proc_sched_param;
@@ -574,28 +635,7 @@ int main(int argc, char **argv)
 	dup2(fileno(logfp), 1);
 	dup2(fileno(logfp), 2);
 
-	prio_min = sched_get_priority_min(sched_policy);
-	prio_max = sched_get_priority_max(sched_policy);
-	if (prio_min < 0  || prio_max < 0) {
-		log_error("sched_get_priority_min|max");
-		fclose(logfp);
-		exit(1);
-	}
-
-	/* rlimit also restricts prio_max */
-	{
-		struct rlimit lim;
-		getrlimit(RLIMIT_RTPRIO, &lim);
-		log("INFO", "RLIMIT_RTPRIO: soft (cur): %ld hard (max): %ld\n",
-			lim.rlim_cur, lim.rlim_max);
-		if (lim.rlim_cur == 0) {
-			log("FAIL", "process is restricted from manipulating priorities.\n");
-			fclose(logfp);
-			exit(2);
-		}
-		if (lim.rlim_cur > prio_max)
-			prio_max = lim.rlim_cur;
-	}
+	set_rtprio_range();
 
 	proc_sched_param.sched_priority = prio_min;
 	if (sched_setscheduler(getpid(), sched_policy,
@@ -604,13 +644,6 @@ int main(int argc, char **argv)
 		fclose(logfp);
 		exit(3);
 	}
-	if (N > (prio_max - prio_min))
-		N = prio_max - prio_min;
-	if (N < 1) {
-		log("FAIL", "Not enough priority levels to run test.\n");
-		fclose(logfp);
-		exit(4);
-	}
 
 	log("INFO", "running test with %d children\n", N);
 
-- 
1.6.3.3

[PATCH 2/4] cr_tests: futex: pi: Factor out release_pi_futex()

[Matt Helsley]

Try to make the control flow of this function easier to analyze.

Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 futex/pi.c |   61 ++++++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/futex/pi.c b/futex/pi.c
index 41f539b..53d4c5d 100644
--- a/futex/pi.c
+++ b/futex/pi.c
@@ -392,6 +392,34 @@ int child_static_priority(int child_num)
 	return prio_min + child_num; /* inverted: + (N - 1 - child_num);*/
 }
 
+void release_pi_futex(int *retries, int *retval, pid_t tid)
+{
+	int pi_val;
+
+	do {
+		/* Release the futex */
+		pi_val = atomic_cmpxchg(pi_futex, tid, 0);
+		if (pi_val != tid) {
+		    switch (do_unlock_contended_pi_futex(*retries)) {
+		    case -1: /* error -- we already logged the details */
+			    *retval = -100;
+			    break;
+		    case 0: /* ok */
+			    break;
+		    case 1: /* try again */
+			    if (retries) {
+				    retries--;
+				    continue;
+			    }
+			    *retval = -101;
+			    break;
+		    }
+		} /* else we were the last to hold the futex */
+	} while(retries);
+
+	log("INFO", "exited the critical section\n");
+}
+
 int kid(void *child_num_as_pointer)
 {
 	pid_t tid = gettid();
@@ -441,7 +469,8 @@ int kid(void *child_num_as_pointer)
 		pi_val = atomic_cmpxchg(pi_futex, 0, tid);
 		if (pi_val == tid) {
 			log("WARN", "found uncontended pi futex.\n");
-			goto release_pi_futex;
+			release_pi_futex(&retries, &retval, tid);
+			goto out;
 		}
 		retval--;
 
@@ -456,7 +485,8 @@ int kid(void *child_num_as_pointer)
 		retval--;
 		if (get_dynamic_priority(tid, &held_prio)) {
 			log("FAIL", "could not get priority.\n");
-			goto release_pi_futex;
+			release_pi_futex(&retries, &retval, tid);
+			goto out;
 		}
 		retval--;
 
@@ -467,7 +497,8 @@ int kid(void *child_num_as_pointer)
 			 * should wake the next highest priority waiter.
 			 */
 			log("FAIL", "Not woken in priority order.\n");
-			goto release_pi_futex;
+			release_pi_futex(&retries, &retval, tid);
+			goto out;
 		}
 		log("PASS", "Woken in priority order.\n");
 		retval = 0;
@@ -499,7 +530,8 @@ int kid(void *child_num_as_pointer)
 			/* Compare our priority to what we set above. */
 			if (get_dynamic_priority(tid, &held_prio)) {
 				retries = 100;
-				goto release_pi_futex;
+				release_pi_futex(&retries, &retval, tid);
+				goto out;
 			}
 			usleep(1000);
 			retries--;
@@ -522,26 +554,7 @@ int kid(void *child_num_as_pointer)
 		}
 	}
 
-release_pi_futex:
-	/* Release the futex */
-	pi_val = atomic_cmpxchg(pi_futex, tid, 0);
-	if (pi_val != tid) {
-	    switch (do_unlock_contended_pi_futex(retries)) {
-	    case -1: /* error -- we already logged the details */
-		    retval = -100;
-		    break;
-	    case 0: /* ok */
-		    break;
-	    case 1: /* try again */
-		    if (retries) {
-			    retries--;
-			    goto release_pi_futex;
-		    }
-		    retval = -101;
-		    break;
-	    }
-	} /* else we were the last to hold the futex */
-	log("INFO", "exited the critical section\n");
+	release_pi_futex(&retries, &retval, tid);
 out:
 	log("INFO", "exiting\n");
 	/* smp_mb() ?? */
-- 
1.6.3.3

[PATCH 3/4] cr_tests: futex: pu: Fix potential livelock

[Matt Helsley]

Fixup flow of release_pi_futex to truly break out of the loop during
an error.

Also, since it's not used, remove the retries parameter from
do_unlock_contended_futex.

Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 futex/pi.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/futex/pi.c b/futex/pi.c
index 53d4c5d..a39964d 100644
--- a/futex/pi.c
+++ b/futex/pi.c
@@ -330,7 +330,7 @@ again:
 	return -1;
 }
 
-int do_unlock_contended_pi_futex(int retries)
+int do_unlock_contended_pi_futex(void)
 {
 	if (futex(&pi_futex->counter, FUTEX_UNLOCK_PI, 1, NULL, NULL, 0) == 0)
 		return 0;
@@ -400,12 +400,12 @@ void release_pi_futex(int *retries, int *retval, pid_t tid)
 		/* Release the futex */
 		pi_val = atomic_cmpxchg(pi_futex, tid, 0);
 		if (pi_val != tid) {
-		    switch (do_unlock_contended_pi_futex(*retries)) {
+		    switch (do_unlock_contended_pi_futex(void)) {
 		    case -1: /* error -- we already logged the details */
 			    *retval = -100;
-			    break;
+			    retries--;
 		    case 0: /* ok */
-			    break;
+			    goto released;
 		    case 1: /* try again */
 			    if (retries) {
 				    retries--;
@@ -416,6 +416,7 @@ void release_pi_futex(int *retries, int *retval, pid_t tid)
 		    }
 		} /* else we were the last to hold the futex */
 	} while(retries);
+released:
 
 	log("INFO", "exited the critical section\n");
 }
-- 
1.6.3.3

[PATCH 4/4] cr_tests: futex: pi: Set the FUTEX_WAITERS bit.

[Matt Helsley]

Need to indicate to the kernel that we're waiting on the futex.
(See kernel source Documentation/pi-futex.txt).

Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 futex/pi.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/futex/pi.c b/futex/pi.c
index a39964d..695eb0a 100644
--- a/futex/pi.c
+++ b/futex/pi.c
@@ -271,6 +271,7 @@ int do_lock_contended_pi_futex(int retries)
 {
 	int do_print = 1;
 
+	__sync_or_and_fetch(&pi_futex->counter, FUTEX_WAITERS);
 again:
 	if (futex(&pi_futex->counter, FUTEX_LOCK_PI, atomic_read(pi_futex),
 	      NULL, NULL, 0) == 0)
-- 
1.6.3.3

Re: [PATCH 3/4] cr_tests: futex: pu: Fix potential livelock

[Serge E. Hallyn]

Quoting Matt Helsley (matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> Fixup flow of release_pi_futex to truly break out of the loop during
> an error.
> 
> Also, since it's not used, remove the retries parameter from
> do_unlock_contended_futex.
> 
> Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  futex/pi.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/futex/pi.c b/futex/pi.c
> index 53d4c5d..a39964d 100644
> --- a/futex/pi.c
> +++ b/futex/pi.c
> @@ -330,7 +330,7 @@ again:
>  	return -1;
>  }
> 
> -int do_unlock_contended_pi_futex(int retries)
> +int do_unlock_contended_pi_futex(void)
>  {
>  	if (futex(&pi_futex->counter, FUTEX_UNLOCK_PI, 1, NULL, NULL, 0) == 0)
>  		return 0;
> @@ -400,12 +400,12 @@ void release_pi_futex(int *retries, int *retval, pid_t tid)
>  		/* Release the futex */
>  		pi_val = atomic_cmpxchg(pi_futex, tid, 0);
>  		if (pi_val != tid) {
> -		    switch (do_unlock_contended_pi_futex(*retries)) {
> +		    switch (do_unlock_contended_pi_futex(void)) {

Just fyi, my compiler didn't like this.  Replacing it with
		    switch (do_unlock_contended_pi_futex()) {

It also doesn't change the failure of robust testcase on my s390 box:

linuz11:~/cr_tests/futex # sh run.sh 
Using output dir ./cr_futex_PR5nlHC
WARNING: Priority inheritance test must be able to set at least two realtime priorities. ulimit -r indicates otherwise so skipping pi futex test(s).
Running test: plain
Test plain done, returned 0
PASS
Restart of test plain done, returned 0
PASS
Running test: robust
run.sh: line 49: kill: (3900) - No such process
run.sh: line 49: kill: (3900) - No such process

Changing the line waiting for the original testcase to
exit from
	wait ${TEST_PID}
to
        wait ${TEST_PID} || true

lets that continue and pass.

(further changing it to
	wait ${TEST_PID} || echo "wait returned $?" && true
gives me
	wait returned 1
while checking process listings before and after the wait
shows that robust is running for a few seconds, and does
die before wait returns, so I assume robust.c is in fact
exiting with error code)

Meanwhile, after ulimit -r 2 pi testcase still just hogs my
cpus :)

-serge