RCU priority boosting that survives semi-vicious testing

This patch adds an optional preemption kernel thread to the rcutorture tests. This thread sets itself to a low RT priority and chews up CPU in 10-second bursts, verifying that grace periods progress during this 10-second interval. This has thus far passed about 30 hours of RCU torture testing on a 4-CPU (a pair of 2-CPU dies) 64-bit Xeon system.

I am experimenting with more-vicious tests, but extra viciousness thus far comes at the expense of grotesque code.

diff -urpNa -X dontdiff linux-2.6.20-rc4-rt1/kernel/rcutorture.c linux-2.6.20-rc4-rt1-rcubtorture/kernel/rcutorture.c

kernel/rcutorture.c +2 -0

@@ -194,6 +194,8 @@ struct rcu_torture_ops {
 	int (*completed)(void);
 	void (*deferredfree)(struct rcu_torture *p);
 	void (*sync)(void);
+	void (*preemptstart)(void);
+	void (*preemptend)(void);
 	int (*stats)(char *page);
 	char *name;
 };

@@ -258,6 +260,71 @@ static void rcu_torture_deferred_free(st
 	call_rcu(&p->rtort_rcu, rcu_torture_cb);

kernel/rcutorture.c +9 -0

+static int rcu_torture_preempt(void *arg)
+{
+	int completedstart;
+	time_t gcstart;
+	struct sched_param sp;
+
+	sp.sched_priority = MAX_RT_PRIO - 1;
+	sched_setscheduler(current, SCHED_RR, &sp);
+	current->flags |= PF_NOFREEZE;

kernel/rcutorture.c +3 -1

 static struct rcu_torture_ops rcu_ops = {
 	.init = NULL,
 	.cleanup = NULL,

@@ -267,7 +334,9 @@ static struct rcu_torture_ops rcu_ops = 
 	.completed = rcu_torture_completed,
 	.deferredfree = rcu_torture_deferred_free,
 	.sync = synchronize_rcu,
-	.stats = NULL,
+	.preemptstart = rcu_preempt_start,
+	.preemptend = rcu_preempt_end,
+	.stats = rcu_preempt_stats,
 	.name = "rcu"
 };
 

Quoted 7 lines · Paul E. McKenney

This patch adds an optional preemption kernel thread to the rcutorture tests. This thread sets itself to a low RT priority and chews up CPU in 10-second bursts, verifying that grace periods progress during this 10-second interval. This has thus far passed about 30 hours of RCU torture testing on a 4-CPU (a pair of 2-CPU dies) 64-bit Xeon system.

Quoted 2 lines

I am experimenting with more-vicious tests, but extra viciousness thus far comes at the expense of grotesque code.

Overall, the new feature seems like a good idea, and it should exercise the new RCU boosting code. Some comments below.

One major item: this new test feature really needs a new module parameter to enable or disable it.

Quoted 16 lines

diff -urpNa -X dontdiff linux-2.6.20-rc4-rt1/kernel/rcutorture.c linux-2.6.20-rc4-rt1-rcubtorture/kernel/rcutorture.c

kernel/rcutorture.c +2 -0

@@ -194,6 +194,8 @@ struct rcu_torture_ops {
 	int (*completed)(void);
 	void (*deferredfree)(struct rcu_torture *p);
 	void (*sync)(void);
+	void (*preemptstart)(void);
+	void (*preemptend)(void);
 	int (*stats)(char *page);
 	char *name;
 };

@@ -258,6 +260,71 @@ static void rcu_torture_deferred_free(st
 	call_rcu(&p->rtort_rcu, rcu_torture_cb);
 }
 

kernel/rcutorture.c static void rcu_torture_deferred_free(st · line 270

Quoted 8 lines

kernel/rcutorture.c +8 -0

+#ifndef CONFIG_PREEMPT_RCU_BOOST
+static void rcu_preempt_start(void) { }
+static void rcu_preempt_end(void) { }
+static int rcu_preempt_stats(char *page) { return 0; }
+#else /* #ifndef CONFIG_PREEMPT_RCU_BOOST */
+
+static struct task_struct *rcu_preeempt_task;
+static long rcu_torture_preempt_errors = 0;

Might as well make this an unsigned long; negative values wouldn't make sense.

Quoted 14 lines

kernel/rcutorture.c +14 -0

+static int rcu_torture_preempt(void *arg)
+{
+	int completedstart;
+	time_t gcstart;
+	struct sched_param sp;
+
+	sp.sched_priority = MAX_RT_PRIO - 1;
+	sched_setscheduler(current, SCHED_RR, &sp);
+	current->flags |= PF_NOFREEZE;
+
+	do {
+		completedstart = rcu_torture_completed();
+		gcstart = xtime.tv_sec;
+		while ((xtime.tv_sec - gcstart < 10) &&

kernel/rcutorture.c static void rcu_torture_deferred_free(st · line 290

Quoted 5 lines

kernel/rcutorture.c +5 -0

+		       (rcu_torture_completed() == completedstart))
+			cond_resched();
+		if (rcu_torture_completed() == completedstart)
+			rcu_torture_preempt_errors++;
+		schedule_timeout_interruptible(shuffle_interval * HZ);

Why call schedule_timeout_interruptible here without actually handling interruptions? So that you can send it a signal to cause the shuffle early?

Quoted 4 lines

kernel/rcutorture.c +4 -0

+	} while (!kthread_should_stop());
+	return NULL;
+}
+

kernel/rcutorture.c static void rcu_torture_deferred_free(st · line 300

Quoted 6 lines

kernel/rcutorture.c +6 -0

+static void rcu_preempt_start(void)
+{
+	rcu_preeempt_task = kthread_run(rcu_torture_preempt, NULL,
+					"rcu_torture_preempt");
+	if (IS_ERR(rcu_preeempt_task)) {
+		VERBOSE_PRINTK_ERRSTRING("Failed to create preempter");

This ought to include the errno value, PTR_ERR(rcu_preempt_task).

kernel/rcutorture.c static void rcu_torture_deferred_free(st · line 307

Quoted 7 lines

kernel/rcutorture.c +7 -0

+		rcu_preeempt_task = NULL;
+	}
+}
+
+static void rcu_preempt_end(void)
+{
+	if (rcu_preeempt_task != NULL) {

if (rcu_preempt_task) would work just as well here.

Quoted 9 lines

kernel/rcutorture.c +9 -0

+		VERBOSE_PRINTK_STRING("Stopping rcu_preempt task");
+		kthread_stop(rcu_preeempt_task);
+	}
+	rcu_preeempt_task = NULL;
+}
+
+static int rcu_preempt_stats(char *page) {
+	int cnt = 0;
+

kernel/rcutorture.c static void rcu_torture_deferred_free(st · line 320

Quoted 4 lines

kernel/rcutorture.c +4 -0

+	cnt += sprintf(&page[cnt],
+		       "Preemption stalls: %ld\n", rcu_torture_preempt_errors);
+	return (cnt);
+}

How about just:
return sprintf(page, ...);
?

Also, if you decide to make rcu_torture_preempt_errors an unsigned long as suggested above, this should use %lu.

kernel/rcutorture.c static void rcu_torture_deferred_free(st · line 327

Quoted 7 lines

kernel/rcutorture.c +7 -0

+#endif /* #else #ifndef CONFIG_PREEMPT_RCU_BOOST */
+
+static void rcu_preemptstart(void)
+{
+	
+}
+

This looks like a bit of stray code.

Quoted 14 lines

kernel/rcutorture.c +3 -1

 static struct rcu_torture_ops rcu_ops = {
 	.init = NULL,
 	.cleanup = NULL,

@@ -267,7 +334,9 @@ static struct rcu_torture_ops rcu_ops = 
 	.completed = rcu_torture_completed,
 	.deferredfree = rcu_torture_deferred_free,
 	.sync = synchronize_rcu,
-	.stats = NULL,
+	.preemptstart = rcu_preempt_start,
+	.preemptend = rcu_preempt_end,
+	.stats = rcu_preempt_stats,
 	.name = "rcu"
 };
 

kernel/rcutorture.c static struct rcu_torture_ops rcu_sync_o · line 382

Quoted 9 lines

kernel/rcutorture.c +2 -0

@@ -306,6 +375,8 @@ static struct rcu_torture_ops rcu_sync_o
 	.completed = rcu_torture_completed,
 	.deferredfree = rcu_sync_torture_deferred_free,
 	.sync = synchronize_rcu,
+	.preemptstart = NULL,
+	.preemptend = NULL,
 	.stats = NULL,
 	.name = "rcu_sync"
 };

Much like other common structures such as struct file_operations, no need to explicitly specify members as NULL here; any member you don't specify will get a NULL value. That avoids the need to update every use of this structure whenever you add a new member used by only some of them.

kernel/rcutorture.c static struct rcu_torture_ops rcu_bh_ops · line 448

Quoted 9 lines

kernel/rcutorture.c +2 -0

@@ -370,6 +441,8 @@ static struct rcu_torture_ops rcu_bh_ops
 	.completed = rcu_bh_torture_completed,
 	.deferredfree = rcu_bh_torture_deferred_free,
 	.sync = rcu_bh_torture_synchronize,
+	.preemptstart = NULL,
+	.preemptend = NULL,
 	.stats = NULL,
 	.name = "rcu_bh"
 };

Likewise.

kernel/rcutorture.c static struct rcu_torture_ops rcu_bh_syn · line 463

Quoted 9 lines

kernel/rcutorture.c +2 -0

@@ -383,6 +456,8 @@ static struct rcu_torture_ops rcu_bh_syn
 	.completed = rcu_bh_torture_completed,
 	.deferredfree = rcu_sync_torture_deferred_free,
 	.sync = rcu_bh_torture_synchronize,
+	.preemptstart = NULL,
+	.preemptend = NULL,
 	.stats = NULL,
 	.name = "rcu_bh_sync"
 };

Likewise.

kernel/rcutorture.c static struct rcu_torture_ops srcu_ops = · line 546

Quoted 9 lines

kernel/rcutorture.c +2 -0

@@ -464,6 +539,8 @@ static struct rcu_torture_ops srcu_ops =
 	.completed = srcu_torture_completed,
 	.deferredfree = rcu_sync_torture_deferred_free,
 	.sync = srcu_torture_synchronize,
+	.preemptstart = NULL,
+	.preemptend = NULL,
 	.stats = srcu_torture_stats,
 	.name = "srcu"
 };

Likewise.

kernel/rcutorture.c static struct rcu_torture_ops sched_ops · line 585

Quoted 8 lines

kernel/rcutorture.c +2 -0

@@ -502,6 +579,8 @@ static struct rcu_torture_ops sched_ops 
 	.completed = sched_torture_completed,
 	.deferredfree = rcu_sync_torture_deferred_free,
 	.sync = sched_torture_synchronize,
+	.preemptstart = NULL,
+	.preemptend = NULL,
 	.stats = NULL,
 	.name = "sched"

Likewise.

kernel/rcutorture.c rcu_torture_cleanup(void) · line 938

Quoted 5 lines

kernel/rcutorture.c +1 -0

@@ -856,6 +935,8 @@ rcu_torture_cleanup(void)
 		kthread_stop(stats_task);
 	}
 	stats_task = NULL;
+	if (cur_ops->preemptend != NULL)

if (cur_ops->preemptend) would work as well.

kernel/rcutorture.c rcu_torture_init(void) · line 1081

Quoted 5 lines

kernel/rcutorture.c +1 -0

@@ -997,6 +1078,8 @@ rcu_torture_init(void)
 			goto unwind;
 		}
 	}
+	if (cur_ops->preemptstart != NULL)

Likewise.

Quoted 2 lines · Josh Triplett

One major item: this new test feature really needs a new module parameter to enable or disable it.

CONFIG_PREEMPT_RCU_BOOST is the parameter -- if not set, then no test. This parameter is provided by the accompanying RCU-boost patch.

Quoted 18 lines

Quoted 17 lines · Paul E. McKenney

diff -urpNa -X dontdiff linux-2.6.20-rc4-rt1/kernel/rcutorture.c linux-2.6.20-rc4-rt1-rcubtorture/kernel/rcutorture.c

kernel/rcutorture.c +13 -0

 
+static int rcu_torture_preempt(void *arg)
+{
+	int completedstart;
+	time_t gcstart;
+	struct sched_param sp;
+
+	sp.sched_priority = MAX_RT_PRIO - 1;
+	sched_setscheduler(current, SCHED_RR, &sp);
+	current->flags |= PF_NOFREEZE;
+
+	do {
+		completedstart = rcu_torture_completed();
+		gcstart = xtime.tv_sec;

Quoted 8 lines

kernel/rcutorture.c static void rcu_torture_deferred_free(st · line 290

Quoted 6 lines

kernel/rcutorture.c +6 -0

+		while ((xtime.tv_sec - gcstart < 10) &&
+		       (rcu_torture_completed() == completedstart))
+			cond_resched();
+		if (rcu_torture_completed() == completedstart)
+			rcu_torture_preempt_errors++;
+		schedule_timeout_interruptible(shuffle_interval * HZ);

Why call schedule_timeout_interruptible here without actually handling interruptions? So that you can send it a signal to cause the shuffle early?

It allows you to kill the process in order to get the module unload to happen more quickly in case someone specified an overly long interval.

But now that you mention this, a simple one-second sleep is probably appropriate here.

Quoted 11 lines

kernel/rcutorture.c static void rcu_torture_deferred_free(st · line 300

Quoted 10 lines

kernel/rcutorture.c +10 -0

+	} while (!kthread_should_stop());
+	return NULL;
+}
+
+static void rcu_preempt_start(void)
+{
+	rcu_preeempt_task = kthread_run(rcu_torture_preempt, NULL,
+					"rcu_torture_preempt");
+	if (IS_ERR(rcu_preeempt_task)) {
+		VERBOSE_PRINTK_ERRSTRING("Failed to create preempter");

This ought to include the errno value, PTR_ERR(rcu_preempt_task).

Good point -- what I should do is return this value so that rcu_torture_init() can return it, failing the module-load process and unwinding.

Quoted 8 lines

kernel/rcutorture.c static void rcu_torture_deferred_free(st · line 307

Quoted 7 lines

kernel/rcutorture.c +7 -0

+		rcu_preeempt_task = NULL;
+	}
+}
+
+static void rcu_preempt_end(void)
+{
+	if (rcu_preeempt_task != NULL) {

if (rcu_preempt_task) would work just as well here.

True, but was being consistent with usage elsewhere in this file.

Quoted 14 lines

Quoted 14 lines

static struct rcu_torture_ops rcu_ops = {

kernel/rcutorture.c +3 -1

 	.init = NULL,
 	.cleanup = NULL,

@@ -267,7 +334,9 @@ static struct rcu_torture_ops rcu_ops = 
 	.completed = rcu_torture_completed,
 	.deferredfree = rcu_torture_deferred_free,
 	.sync = synchronize_rcu,
-	.stats = NULL,
+	.preemptstart = rcu_preempt_start,
+	.preemptend = rcu_preempt_end,
+	.stats = rcu_preempt_stats,
 	.name = "rcu"
 };
 

Quoted 13 lines

kernel/rcutorture.c static struct rcu_torture_ops rcu_sync_o · line 382

Quoted 9 lines

kernel/rcutorture.c +2 -0

@@ -306,6 +375,8 @@ static struct rcu_torture_ops rcu_sync_o
 	.completed = rcu_torture_completed,
 	.deferredfree = rcu_sync_torture_deferred_free,
 	.sync = synchronize_rcu,
+	.preemptstart = NULL,
+	.preemptend = NULL,
 	.stats = NULL,
 	.name = "rcu_sync"
 };

Much like other common structures such as struct file_operations, no need to explicitly specify members as NULL here; any member you don't specify will get a NULL value. That avoids the need to update every use of this structure whenever you add a new member used by only some of them.

Untrusting, aren't I? ;-)

I removed all the "= NULL" entries.

Quoted 6 lines

kernel/rcutorture.c rcu_torture_cleanup(void) · line 938

Quoted 5 lines

kernel/rcutorture.c +1 -0

@@ -856,6 +935,8 @@ rcu_torture_cleanup(void)
 		kthread_stop(stats_task);
 	}
 	stats_task = NULL;
+	if (cur_ops->preemptend != NULL)

if (cur_ops->preemptend) would work as well.

True, though again there is a lot of existing "!= NULL" in this file and elsewhere. Many thousands of them through the kernel. ;-)

I will run this through the mill and repost.

Quoted 6 lines · Paul E. McKenney

Quoted 2 lines · Josh Triplett

One major item: this new test feature really needs a new module parameter to enable or disable it.

CONFIG_PREEMPT_RCU_BOOST is the parameter -- if not set, then no test. This parameter is provided by the accompanying RCU-boost patch.

It seems useful for rcutorture to use or not use the preempting thread independently of CONFIG_PREEMPT_RCU_BOOST. That would bring you from two cases to four, and the two new cases both make sense:

* CONFIG_PREEMPT_RCU_BOOST=n, but run rcutorture with the preempting thread. This configuration allows you to demonstrate the need for CONFIG_PREEMPT_RCU_BOOST, by showing what happens when you need it and don't have it.

* CONFIG_PREEMPT_RCU_BOOST=y, but run rcutorture without the preempting thread. This configuration allows you to test with rcutorture while running a *real* real-time workload rather than the simple preempting thread, or just test basic RCU functionality.

A simple boolean module_param would work here.

At some point, we may want to add the ability to run multiple preempting threads, but that doesn't need to happen for this patch.

Quoted 27 lines

Quoted 26 lines

Quoted 13 lines · Paul E. McKenney

diff -urpNa -X dontdiff linux-2.6.20-rc4-rt1/kernel/rcutorture.c linux-2.6.20-rc4-rt1-rcubtorture/kernel/rcutorture.c

kernel/rcutorture.c +9 -0

 
+static int rcu_torture_preempt(void *arg)
+{
+	int completedstart;
+	time_t gcstart;
+	struct sched_param sp;
+
+	sp.sched_priority = MAX_RT_PRIO - 1;
+	sched_setscheduler(current, SCHED_RR, &sp);
+	current->flags |= PF_NOFREEZE;

kernel/rcutorture.c static void rcu_torture_deferred_free(st · line 290

Quoted 10 lines

kernel/rcutorture.c +10 -0

+
+	do {
+		completedstart = rcu_torture_completed();
+		gcstart = xtime.tv_sec;
+		while ((xtime.tv_sec - gcstart < 10) &&
+		       (rcu_torture_completed() == completedstart))
+			cond_resched();
+		if (rcu_torture_completed() == completedstart)
+			rcu_torture_preempt_errors++;
+		schedule_timeout_interruptible(shuffle_interval * HZ);

Why call schedule_timeout_interruptible here without actually handling interruptions? So that you can send it a signal to cause the shuffle early?

Quoted 2 lines

It allows you to kill the process in order to get the module unload to happen more quickly in case someone specified an overly long interval.

I didn't actually know that you could kill a kthread from userspace. :)

That rationale makes sense.

Quoted 2 lines

But now that you mention this, a simple one-second sleep is probably appropriate here.

OK.

Quoted 12 lines

Quoted 11 lines

kernel/rcutorture.c static void rcu_torture_deferred_free(st · line 300

Quoted 10 lines

kernel/rcutorture.c +10 -0

+	} while (!kthread_should_stop());
+	return NULL;
+}
+
+static void rcu_preempt_start(void)
+{
+	rcu_preeempt_task = kthread_run(rcu_torture_preempt, NULL,
+					"rcu_torture_preempt");
+	if (IS_ERR(rcu_preeempt_task)) {
+		VERBOSE_PRINTK_ERRSTRING("Failed to create preempter");

This ought to include the errno value, PTR_ERR(rcu_preempt_task).

Quoted 3 lines

Good point -- what I should do is return this value so that rcu_torture_init() can return it, failing the module-load process and unwinding.

Even better, yes.

Quoted 9 lines

Quoted 8 lines

kernel/rcutorture.c static void rcu_torture_deferred_free(st · line 307

Quoted 7 lines

kernel/rcutorture.c +7 -0

+		rcu_preeempt_task = NULL;
+	}
+}
+
+static void rcu_preempt_end(void)
+{
+	if (rcu_preeempt_task != NULL) {

if (rcu_preempt_task) would work just as well here.

Quoted 1 line

True, but was being consistent with usage elsewhere in this file.

Fair enough; don't worry about it for this patch, then. I'll deal with that particular style cleanup later, throughout rcutorture.

Quoted 28 lines

Quoted 14 lines

Quoted 14 lines

static struct rcu_torture_ops rcu_ops = {

kernel/rcutorture.c +3 -1

 	.init = NULL,
 	.cleanup = NULL,

@@ -267,7 +334,9 @@ static struct rcu_torture_ops rcu_ops = 
 	.completed = rcu_torture_completed,
 	.deferredfree = rcu_torture_deferred_free,
 	.sync = synchronize_rcu,
-	.stats = NULL,
+	.preemptstart = rcu_preempt_start,
+	.preemptend = rcu_preempt_end,
+	.stats = rcu_preempt_stats,
 	.name = "rcu"
 };
 

Quoted 13 lines

kernel/rcutorture.c static struct rcu_torture_ops rcu_sync_o · line 382

Quoted 9 lines

kernel/rcutorture.c +2 -0

@@ -306,6 +375,8 @@ static struct rcu_torture_ops rcu_sync_o
 	.completed = rcu_torture_completed,
 	.deferredfree = rcu_sync_torture_deferred_free,
 	.sync = synchronize_rcu,
+	.preemptstart = NULL,
+	.preemptend = NULL,
 	.stats = NULL,
 	.name = "rcu_sync"
 };

Much like other common structures such as struct file_operations, no need to explicitly specify members as NULL here; any member you don't specify will get a NULL value. That avoids the need to update every use of this structure whenever you add a new member used by only some of them.

Quoted 1 line

Untrusting, aren't I? ;-)

Heh. I have that problem as well; I always hestitate to trust the compiler to initialize values.

Quoted 1 line

I removed all the "= NULL" entries.

Thanks.

Quoted 7 lines

Quoted 6 lines

kernel/rcutorture.c rcu_torture_cleanup(void) · line 938

Quoted 5 lines

kernel/rcutorture.c +1 -0

@@ -856,6 +935,8 @@ rcu_torture_cleanup(void)
 		kthread_stop(stats_task);
 	}
 	stats_task = NULL;
+	if (cur_ops->preemptend != NULL)

if (cur_ops->preemptend) would work as well.

Quoted 2 lines

True, though again there is a lot of existing "!= NULL" in this file and elsewhere. Many thousands of them through the kernel. ;-)

As before, don't worry about it for this patch then.

Quoted 1 line

I will run this through the mill and repost.

Quoted 15 lines · Josh Triplett

Quoted 5 lines · Paul E. McKenney

Quoted 2 lines · Josh Triplett

One major item: this new test feature really needs a new module parameter to enable or disable it.

CONFIG_PREEMPT_RCU_BOOST is the parameter -- if not set, then no test. This parameter is provided by the accompanying RCU-boost patch.

It seems useful for rcutorture to use or not use the preempting thread independently of CONFIG_PREEMPT_RCU_BOOST. That would bring you from two cases to four, and the two new cases both make sense:

* CONFIG_PREEMPT_RCU_BOOST=n, but run rcutorture with the preempting thread. This configuration allows you to demonstrate the need for CONFIG_PREEMPT_RCU_BOOST, by showing what happens when you need it and don't have it.

Quoted 6 lines

* CONFIG_PREEMPT_RCU_BOOST=y, but run rcutorture without the preempting thread. This configuration allows you to test with rcutorture while running a *real* real-time workload rather than the simple preempting thread, or just test basic RCU functionality.

A simple boolean module_param would work here.

OK, sold! I will add this. Perhaps CONFIG_PREEMPT_RCU_TORTURE.

Quoted 27 lines

Quoted 27 lines

Quoted 17 lines

Quoted 16 lines · Paul E. McKenney

diff -urpNa -X dontdiff linux-2.6.20-rc4-rt1/kernel/rcutorture.c linux-2.6.20-rc4-rt1-rcubtorture/kernel/rcutorture.c

kernel/rcutorture.c +13 -0

+static int rcu_torture_preempt(void *arg)
+{
+	int completedstart;
+	time_t gcstart;
+	struct sched_param sp;
+
+	sp.sched_priority = MAX_RT_PRIO - 1;
+	sched_setscheduler(current, SCHED_RR, &sp);
+	current->flags |= PF_NOFREEZE;
+
+	do {
+		completedstart = rcu_torture_completed();
+		gcstart = xtime.tv_sec;

Quoted 8 lines

kernel/rcutorture.c static void rcu_torture_deferred_free(st · line 290

Quoted 6 lines

kernel/rcutorture.c +6 -0

+		while ((xtime.tv_sec - gcstart < 10) &&
+		       (rcu_torture_completed() == completedstart))
+			cond_resched();
+		if (rcu_torture_completed() == completedstart)
+			rcu_torture_preempt_errors++;
+		schedule_timeout_interruptible(shuffle_interval * HZ);

Why call schedule_timeout_interruptible here without actually handling interruptions? So that you can send it a signal to cause the shuffle early?

It allows you to kill the process in order to get the module unload to happen more quickly in case someone specified an overly long interval.

Quoted 3 lines

I didn't actually know that you could kill a kthread from userspace. :)

That rationale makes sense.

It won't actually die, but if I understand correctly (a big "if") the signal would cause schedule_timeout_interruptible() to return, allowing the kthread_should_stop() check to happen.

Quoted 6 lines · Paul E. McKenney

Quoted 2 lines · Josh Triplett

One major item: this new test feature really needs a new module parameter to enable or disable it.

CONFIG_PREEMPT_RCU_BOOST is the parameter -- if not set, then no test. This parameter is provided by the accompanying RCU-boost patch.

It seems useful for rcutorture to use or not use the preempting thread independently of CONFIG_PREEMPT_RCU_BOOST. That would bring you from two cases to four, and the two new cases both make sense:

* CONFIG_PREEMPT_RCU_BOOST=n, but run rcutorture with the preempting thread. This configuration allows you to demonstrate the need for CONFIG_PREEMPT_RCU_BOOST, by showing what happens when you need it and don't have it.

* CONFIG_PREEMPT_RCU_BOOST=y, but run rcutorture without the preempting thread. This configuration allows you to test with rcutorture while running a *real* real-time workload rather than the simple preempting thread, or just test basic RCU functionality.

A simple boolean module_param would work here.

At some point, we may want to add the ability to run multiple preempting threads, but that doesn't need to happen for this patch.

Quoted 15 lines · Josh Triplett

Quoted 5 lines · Paul E. McKenney

Quoted 2 lines · Josh Triplett

One major item: this new test feature really needs a new module parameter to enable or disable it.

CONFIG_PREEMPT_RCU_BOOST is the parameter -- if not set, then no test. This parameter is provided by the accompanying RCU-boost patch.

It seems useful for rcutorture to use or not use the preempting thread independently of CONFIG_PREEMPT_RCU_BOOST. That would bring you from two cases to four, and the two new cases both make sense:

* CONFIG_PREEMPT_RCU_BOOST=n, but run rcutorture with the preempting thread. This configuration allows you to demonstrate the need for CONFIG_PREEMPT_RCU_BOOST, by showing what happens when you need it and don't have it.

Quoted 6 lines

* CONFIG_PREEMPT_RCU_BOOST=y, but run rcutorture without the preempting thread. This configuration allows you to test with rcutorture while running a *real* real-time workload rather than the simple preempting thread, or just test basic RCU functionality.

A simple boolean module_param would work here.

OK, am finally with you. See below for updated patch.

Signed-off-by Paul E. McKenney <[email protected]>

include/linux/rcupreempt.h +0 -0

diff -urpNa -X dontdiff linux-2.6.20-rc4-rt1/kernel/rcupreempt.c linux-2.6.20-rc4-rt1-rcub/kernel/rcupreempt.c

kernel/rcupreempt.c +0 -0

@@ -49,6 +49,7 @@

kernel/rcupreempt.c +4 -0

+#ifdef CONFIG_PREEMPT_RCU_BOOST_STATS
+
+/*
+ * Function to increment indicated ->rbs_stats[] element.

kernel/rcupreempt.c +5 -0

+#define rcu_boost_dat_stat_block(rbdp, oldstate) \
+	rcu_boost_dat_stat(rbdp, RCU_BOOST_DAT_BLOCK, oldstate)
+#define rcu_boost_dat_stat_boost(rbdp, oldstate) \
+	rcu_boost_dat_stat(rbdp, RCU_BOOST_DAT_BOOST, oldstate)
+#define rcu_boost_dat_stat_unlock(rbdp, oldstate) \

kernel/rcupreempt.c +8 -0

+	/* Print them out! */
+
+	printk(KERN_ALERT
+	       "rcu_boost_dat: idx=%d "
+	       "b=%ld ul=%ld ub=%ld boost: a=%ld b=%ld\n",
+	       rcu_boost_idx,
+	       sum.rbs_blocked, sum.rbs_unlock, sum.rbs_unboosted,
+	       sum.rbs_boost_attempt, sum.rbs_boost);

kernel/rcupreempt.c +5 -0

+	/* Go away and don't come back for awhile. */
+
+	lastprint = xtime.tv_sec;
+}
+

kernel/rcupreempt.c +23 -0

+/*
+ * Initialize RCU-boost state.  This happens early in the boot process,
+ * when the scheduler does not yet exist.  So don't try to use it.
+ */
+static void init_rcu_boost_early(void)
+{
+	struct rcu_boost_dat *rbdp;
+	int cpu;
+	int i;
+
+	for_each_possible_cpu(cpu) {
+		rbdp = per_cpu(rcu_boost_dat, cpu);
+		for (i = 0; i < RCU_BOOST_ELEMENTS; i++) {
+			rbdp[i].rbs_mutex =
+				RAW_SPIN_LOCK_UNLOCKED(rbdp[i].rbs_mutex);
+			INIT_LIST_HEAD(&rbdp[i].rbs_toboost);
+			INIT_LIST_HEAD(&rbdp[i].rbs_boosted);
+			rbdp[i].rbs_blocked = 0;
+			rbdp[i].rbs_boost_attempt = 0;
+			rbdp[i].rbs_boost = 0;
+			rbdp[i].rbs_unlock = 0;
+			rbdp[i].rbs_unboosted = 0;
+#ifdef CONFIG_PREEMPT_RCU_BOOST_STATS

kernel/rcupreempt.c +3 -0

+		rcu_boost_idx = 0;
+	}
+}
 

kernel/rcupreempt.c +27 -0

+}
 
+/*
+ * Priority-boost tasks stuck in RCU read-side critical sections as
+ * needed (presumably rarely).
+ */
+static int rcu_booster(void *arg)
+{
+	int cpu;
+	struct sched_param sp;
+
+	sp.sched_priority = PREEMPT_RCU_BOOSTER_PRIO;
+	sched_setscheduler(current, SCHED_RR, &sp);
+	current->flags |= PF_NOFREEZE;
+
+	do {
+
+		/* Advance the lists of tasks. */
+
+		rcu_boost_idx = (rcu_boost_idx + 1) % RCU_BOOST_ELEMENTS;
+		for_each_possible_cpu(cpu) {
+
+			/*
+			 * Boost all sufficiently aged readers.
+			 * Readers must first be preempted or block
+			 * on a mutex in an RCU read-side critical section,
+			 * then remain in that critical section for
+			 * RCU_BOOST_ELEMENTS-1 time intervals.

kernel/rcupreempt.c +20 -0

+		}
+
+		/*
+		 * Sleep to allow any unstalled RCU read-side critical
+		 * sections to age out of the list.  @@@ FIXME: reduce,
+		 * adjust, or eliminate in case of OOM.
+		 */
+
+		schedule_timeout_uninterruptible(HZ / 100);
+
+		/* Print stats if enough time has passed. */
+
+		rcu_boost_dat_stat_print();
+
+	} while (!kthread_should_stop());
+
+	return 0;
+}
+
+/*

kernel/rcupreempt.c +19 -0

+
+		/*
+		 * Continue running, but tasks permanently blocked
+		 * in RCU read-side critical sections will be able
+		 * to stall grace-period processing, potentially
+		 * OOMing the machine.
+		 */
+
+		rcu_boost_task = NULL;
+	}
+}
+
+/*
+ * Update task's RCU-boost state to reflect blocking in RCU read-side
+ * critical section, so that the RCU-boost task can find it in case it
+ * later needs its priority boosted.
+ */
+void __rcu_preempt_boost(void)
+{

kernel/rcupreempt.c +17 -0

+		return;
+	}
+	spin_lock(&rbdp->rbs_mutex);
+	rbdp->rbs_blocked++;
+
+	/*
+	 * Update state.  We hold the lock and aren't yet on the list,
+	 * so the booster cannot mess with us yet.
+	 */
+
+	rcu_boost_dat_stat_block(rbdp, current->rcub_state);
+	if (current->rcub_state != RCU_BOOST_IDLE) {
+
+		/*
+		 * We have been here before, so just update stats.
+		 * It may seem strange to do all this work just to
+		 * accumulate statistics, but this is such a

kernel/rcupreempt.c +5 -0

+
+	list_add_tail(&current->rcub_entry, &rbdp->rbs_toboost);
+	current->rcub_rbdp = rbdp;
+	spin_unlock_irqrestore(&rbdp->rbs_mutex, oldirq);
+}

diff -urpNa -X dontdiff linux-2.6.20-rc4-rt1/kernel/rtmutex.c linux-2.6.20-rc4-rt1-rcub/kernel/rtmutex.c

Overall, this code looks sensible to me. Some comments on the patch below.

Quoted 2 lines · Paul E. McKenney

include/linux/rcupreempt.h +0 -0

[...]

include/linux/rcupreempt.h line 60

Quoted 7 lines

include/linux/rcupreempt.h +7 -0

+enum rcu_boost_state {
+	RCU_BOOST_IDLE = 0,	   /* Not yet blocked if in RCU read-side. */
+	RCU_BOOST_BLOCKED = 1,	   /* Blocked from RCU read-side. */
+	RCU_BOOSTED = 2,	   /* Boosting complete. */
+};
+
+#define N_RCU_BOOST_STATE (RCU_BOOSTED + 1)

Here, you define the three possible states, the number of states. But later...

Quoted 4 lines

diff -urpNa -X dontdiff linux-2.6.20-rc4-rt1/kernel/rcupreempt.c linux-2.6.20-rc4-rt1-rcub/kernel/rcupreempt.c

kernel/rcupreempt.c +0 -0

@@ -49,6 +49,7 @@

[...]

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 109

Quoted 3 lines

kernel/rcupreempt.c +3 -0

+#ifdef CONFIG_PREEMPT_RCU_BOOST_STATS
+	long rbs_stats[N_RCU_BOOST_DAT_EVENTS][N_RCU_BOOST_STATE + 1];
+#endif /* #ifdef CONFIG_PREEMPT_RCU_BOOST_STATS */

...you declare this array to have space for one more than that, and then...

Quoted 12 lines

kernel/rcupreempt.c +12 -0

+#ifdef CONFIG_PREEMPT_RCU_BOOST_STATS
+
+/*
+ * Function to increment indicated ->rbs_stats[] element.
+ */
+static inline void rcu_boost_dat_stat(struct rcu_boost_dat *rbdp,
+				      int event,
+				      enum rcu_boost_state oldstate)
+{
+	if (oldstate >= RCU_BOOST_IDLE &&
+	    oldstate <= RCU_BOOSTED) {
+		rbdp->rbs_stats[event][oldstate]++;

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 130

Quoted 2 lines

kernel/rcupreempt.c +2 -0

+	} else {
+		rbdp->rbs_stats[event][N_RCU_BOOST_STATE]++;

...you use the last element as what looks like an "unknown" counter. How about instead defining the enum to have an "unknown" state, defining N_RCU_BOOST_STATE accordingly, and using N_RCU_BOOST_STATE without the +1 in rbs_stats? I think that would make the code much more self-documenting, and less error-prone.

Quoted 10 lines

kernel/rcupreempt.c +10 -0

+#define rcu_boost_dat_stat_block(rbdp, oldstate) \
+	rcu_boost_dat_stat(rbdp, RCU_BOOST_DAT_BLOCK, oldstate)
+#define rcu_boost_dat_stat_boost(rbdp, oldstate) \
+	rcu_boost_dat_stat(rbdp, RCU_BOOST_DAT_BOOST, oldstate)
+#define rcu_boost_dat_stat_unlock(rbdp, oldstate) \
+	rcu_boost_dat_stat(rbdp, RCU_BOOST_DAT_UNLOCK, oldstate)
+
+/*
+ * Prefix for kprint() strings for periodic statistics messages.
+ */

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 148

Quoted 5 lines

kernel/rcupreempt.c +5 -0

+static char *rcu_boost_state_event[] = {
+	"block:  ",
+	"boost:  ",
+	"unlock: ",
+};

I don't know for sure, but I *think* that GCC may sometimes generate more efficient code if you use a char [][], rather than a char *[].

Quoted 7 lines

kernel/rcupreempt.c +7 -0

+
+/*
+ * Indicators for numbers in kprint() strings.  "!" indicates a state-event
+ * pair that should not happen, while "?" indicates a state that should
+ * not happen.
+ */
+static char *rcu_boost_state_error[] = {

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 160

Quoted 5 lines

kernel/rcupreempt.c +5 -0

+       /*ibBe*/
+	"   ?",  /* block */
+	"!  ?",  /* boost */
+	"?  ?",  /* unlock */
+};

Likewise. Also, the columns here don't seem to line up, probably because the comment uses spaces and the other lines use a tab.

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 167

Quoted 3 lines

kernel/rcupreempt.c +3 -0

+static void rcu_boost_dat_stat_print(void)
+{
+	char buf[N_RCU_BOOST_STATE * (sizeof(long) * 3 + 2) + 2];

This expression really begs for some constants in place of the magic numbers, or failing that, a comment.

Quoted 9 lines

kernel/rcupreempt.c +9 -0

+	int cpu;
+	int event;
+	int i;
+	static time_t lastprint = 0;
+	struct rcu_boost_dat *rbdp;
+	int state;
+	struct rcu_boost_dat sum;
+
+	/* Wait a graceful interval between printk spamming. */

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 180

Quoted 4 lines

kernel/rcupreempt.c +4 -0

+
+	if (xtime.tv_sec - lastprint <
+	    CONFIG_PREEMPT_RCU_BOOST_STATS_INTERVAL)
+		return;

How about printk_ratelimit? It takes a parameter for the rate. (On the other hand, it also takes a spinlock, which you might not want here, even in the stats code.)

Quoted 13 lines

kernel/rcupreempt.c +13 -0

+	/* Print them out! */
+
+	printk(KERN_ALERT
+	       "rcu_boost_dat: idx=%d "
+	       "b=%ld ul=%ld ub=%ld boost: a=%ld b=%ld\n",
+	       rcu_boost_idx,
+	       sum.rbs_blocked, sum.rbs_unlock, sum.rbs_unboosted,
+	       sum.rbs_boost_attempt, sum.rbs_boost);
+	for (event = 0; event < N_RCU_BOOST_DAT_EVENTS; event++) {
+		i = 0;
+		for (state = 0; state <= N_RCU_BOOST_STATE; state++) {
+			i += sprintf(&buf[i], " %ld%c",
+				     sum.rbs_stats[event][state],

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 230

Quoted 5 lines

kernel/rcupreempt.c +5 -0

+				     rcu_boost_state_error[event][state]);
+		}
+		printk(KERN_ALERT "rcu_boost_dat %s %s\n",
+		       rcu_boost_state_event[event], buf);
+	}

Most or all of these counters could likely become unsigned, since negative values don't make sense for them.

Quoted 8 lines

kernel/rcupreempt.c +8 -0

+	/* Go away and don't come back for awhile. */
+
+	lastprint = xtime.tv_sec;
+}
+
+/*
+ * Return the current boost index for boosting target tasks.
+ * May only be invoked by the booster task, so guaranteed to

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 291

Quoted 6 lines

+ * already be initialized.
+ */

kernel/rcupreempt.c +4 -0

+static inline int rcu_boost_idx_boosting(void)
+{
+	return (rcu_boost_idx + 1) & (RCU_BOOST_ELEMENTS - 1);
+}

Quoted for later reference.

Quoted 29 lines

kernel/rcupreempt.c +29 -0

+/*
+ * Initialize RCU-boost state.  This happens early in the boot process,
+ * when the scheduler does not yet exist.  So don't try to use it.
+ */
+static void init_rcu_boost_early(void)
+{
+	struct rcu_boost_dat *rbdp;
+	int cpu;
+	int i;
+
+	for_each_possible_cpu(cpu) {
+		rbdp = per_cpu(rcu_boost_dat, cpu);
+		for (i = 0; i < RCU_BOOST_ELEMENTS; i++) {
+			rbdp[i].rbs_mutex =
+				RAW_SPIN_LOCK_UNLOCKED(rbdp[i].rbs_mutex);
+			INIT_LIST_HEAD(&rbdp[i].rbs_toboost);
+			INIT_LIST_HEAD(&rbdp[i].rbs_boosted);
+			rbdp[i].rbs_blocked = 0;
+			rbdp[i].rbs_boost_attempt = 0;
+			rbdp[i].rbs_boost = 0;
+			rbdp[i].rbs_unlock = 0;
+			rbdp[i].rbs_unboosted = 0;
+#ifdef CONFIG_PREEMPT_RCU_BOOST_STATS
+			{
+				int j, k;
+
+				for (j = 0; j < N_RCU_BOOST_DAT_EVENTS; j++)
+					for (k = 0; k <= N_RCU_BOOST_STATE; k++)
+						rbdp[i].rbs_stats[j][k] = 0;

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 278

Quoted 4 lines

kernel/rcupreempt.c +4 -0

+			}
+#endif /* #ifdef CONFIG_PREEMPT_RCU_BOOST_STATS */
+		}
+		smp_wmb();

Barrier without an accompanying comment. You want to make sure the structure gets initialized before setting rcu_boost_idx and thus allowing boosting, right?

Quoted 3 lines

kernel/rcupreempt.c +3 -0

+		rcu_boost_idx = 0;
+	}
+}

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 302

Quoted 10 lines

kernel/rcupreempt.c +10 -0

+/*
+ * Return the list on which the calling task should add itself, or
+ * NULL if too early during initialization.
+ */
+static inline struct rcu_boost_dat *rcu_rbd_new(void)
+{
+	int cpu = raw_smp_processor_id();  /* locks used, so preemption OK. */
+	int idx = rcu_boost_idx;
+
+	smp_read_barrier_depends(); barrier();

Barriers without accompanying comments, though in this case they seem fairly obvious.

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 315

Quoted 13 lines

kernel/rcupreempt.c +13 -0

+	if (unlikely(idx < 0))
+		return (NULL);
+	return &per_cpu(rcu_boost_dat, cpu)[idx];
+}
+
+/*
+ * Return the list from which to boost target tasks.
+ * May only be invoked by the booster task, so guaranteed to
+ * already be initialized.
+ */
+static inline struct rcu_boost_dat *rcu_rbd_boosting(int cpu)
+{
+	int idx = (rcu_boost_idx + 1) & (RCU_BOOST_ELEMENTS - 1);

You defined rcu_boost_idx_boosting for exactly this expression.

kernel/rcupreempt.c

Quoted 1 line

kernel/rcupreempt.c +1 -0

+	return &per_cpu(rcu_boost_dat, cpu)[idx];

For that matter, since you wrapped it in the function, you could just call it directly in this array index operation.

Quoted 35 lines

kernel/rcupreempt.c +34 -0

+}
 
+/*
+ * Priority-boost tasks stuck in RCU read-side critical sections as
+ * needed (presumably rarely).
+ */
+static int rcu_booster(void *arg)
+{
+	int cpu;
+	struct sched_param sp;
+
+	sp.sched_priority = PREEMPT_RCU_BOOSTER_PRIO;
+	sched_setscheduler(current, SCHED_RR, &sp);
+	current->flags |= PF_NOFREEZE;
+
+	do {
+
+		/* Advance the lists of tasks. */
+
+		rcu_boost_idx = (rcu_boost_idx + 1) % RCU_BOOST_ELEMENTS;
+		for_each_possible_cpu(cpu) {
+
+			/*
+			 * Boost all sufficiently aged readers.
+			 * Readers must first be preempted or block
+			 * on a mutex in an RCU read-side critical section,
+			 * then remain in that critical section for
+			 * RCU_BOOST_ELEMENTS-1 time intervals.
+			 * So most of the time we should end up doing
+			 * nothing.
+			 */
+
+			rcu_boost_one_reader_list(rcu_rbd_boosting(cpu));
+
+			/*

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 467

Quoted 3 lines

kernel/rcupreempt.c +3 -0

+			 * Large SMP systems may need to sleep sometimes
+			 * in this loop.  Or have multiple RCU-boost tasks.
+			 */

The idea of multiple RCU-boost tasks gives me an idea: what about having per-CPU boost queues, and using that to eliminate the need to lock the queues? It might not do quite as good a job of scheduling, but it would eliminate locking overhead *and* solve the problem you describe here.

Quoted 25 lines

kernel/rcupreempt.c +25 -0

+		}
+
+		/*
+		 * Sleep to allow any unstalled RCU read-side critical
+		 * sections to age out of the list.  @@@ FIXME: reduce,
+		 * adjust, or eliminate in case of OOM.
+		 */
+
+		schedule_timeout_uninterruptible(HZ / 100);
+
+		/* Print stats if enough time has passed. */
+
+		rcu_boost_dat_stat_print();
+
+	} while (!kthread_should_stop());
+
+	return 0;
+}
+
+/*
+ * Perform the portions of RCU-boost initialization that require the
+ * scheduler to be up and running.
+ */
+void init_rcu_boost_late(void)
+{

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 497

Quoted 5 lines

kernel/rcupreempt.c +5 -0

+	int i;
+
+	/* Spawn RCU-boost task. */
+
+	printk(KERN_ALERT "Starting RCU priority booster\n");

Judging by other initialization code, I think this belongs at KERN_INFO or similar.

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 502

Quoted 5 lines

kernel/rcupreempt.c +5 -0

+	rcu_boost_task = kthread_run(rcu_booster, NULL, "RCU Prio Booster");
+	if (IS_ERR(rcu_boost_task)) {
+		i = PTR_ERR(rcu_boost_task);
+		printk(KERN_ALERT
+		       "Unable to create RCU Priority Booster, errno %d\n", -i);

No need for a temporary here.

Quoted 23 lines

kernel/rcupreempt.c +23 -0

+
+		/*
+		 * Continue running, but tasks permanently blocked
+		 * in RCU read-side critical sections will be able
+		 * to stall grace-period processing, potentially
+		 * OOMing the machine.
+		 */
+
+		rcu_boost_task = NULL;
+	}
+}
+
+/*
+ * Update task's RCU-boost state to reflect blocking in RCU read-side
+ * critical section, so that the RCU-boost task can find it in case it
+ * later needs its priority boosted.
+ */
+void __rcu_preempt_boost(void)
+{
+	struct rcu_boost_dat *rbdp;
+	unsigned long oldirq;
+
+	/* Identify list to place task on for possible later boosting. */

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 531

Quoted 6 lines

kernel/rcupreempt.c +6 -0

+
+	local_irq_save(oldirq);
+	rbdp = rcu_rbd_new();
+	if (rbdp == NULL) {
+		local_irq_restore(oldirq);
+		printk("Preempted RCU read-side critical section too early.\n");

This printk lacks a priority.

Quoted 21 lines

kernel/rcupreempt.c +21 -0

+		return;
+	}
+	spin_lock(&rbdp->rbs_mutex);
+	rbdp->rbs_blocked++;
+
+	/*
+	 * Update state.  We hold the lock and aren't yet on the list,
+	 * so the booster cannot mess with us yet.
+	 */
+
+	rcu_boost_dat_stat_block(rbdp, current->rcub_state);
+	if (current->rcub_state != RCU_BOOST_IDLE) {
+
+		/*
+		 * We have been here before, so just update stats.
+		 * It may seem strange to do all this work just to
+		 * accumulate statistics, but this is such a
+		 * low-probability code path that we shouldn't care.
+		 * If it becomes a problem, it can be fixed.
+		 */
+

kernel/rcupreempt.c static struct rcu_ctrlblk rcu_ctrlblk = · line 558

Quoted 6 lines

kernel/rcupreempt.c +6 -0

+		spin_unlock_irqrestore(&rbdp->rbs_mutex, oldirq);
+		return;
+	}
+	current->rcub_state = RCU_BOOST_BLOCKED;
+
+	/* Now add ourselves to the list so that the booster can find use. */

Typo: s/use/us/

Quoted 5 lines

kernel/rcupreempt.c +5 -0

+
+	list_add_tail(&current->rcub_entry, &rbdp->rbs_toboost);
+	current->rcub_rbdp = rbdp;
+	spin_unlock_irqrestore(&rbdp->rbs_mutex, oldirq);
+}

kernel/rtmutex.c static inline void init_lists(struct rt_ · line 111

Quoted 11 lines

diff -urpNa -X dontdiff linux-2.6.20-rc4-rt1/kernel/rtmutex.c linux-2.6.20-rc4-rt1-rcub/kernel/rtmutex.c

kernel/rtmutex.c +4 -0

@@ -105,11 +105,14 @@ static inline void init_lists(struct rt_
  */
 int rt_mutex_getprio(struct task_struct *task)
 {
+	int prio = task->normal_prio;
+
+	if (get_rcu_prio(task) < prio)
+		prio = get_rcu_prio(task);

int prio = min(task->normal_prio, get_rcu_prio(task)); ?

kernel/rtmutex.c static inline void init_lists(struct rt_ · line 116

Quoted 8 lines

kernel/rtmutex.c +2 -3

 	if (likely(!task_has_pi_waiters(task)))
-		return task->normal_prio;
+		return prio;
 
-	return min(task_top_pi_waiter(task)->pi_list_entry.prio,
-		   task->normal_prio);
+	return min(task_top_pi_waiter(task)->pi_list_entry.prio, prio);
 }

Quoted 1 line · Josh Triplett

Overall, this code looks sensible to me. Some comments on the patch below.

Thank you again for the careful and thorough review!!!

I will test these changes and send out an update.

Quoted 2 lines · Paul E. McKenney

Quoted 1 line · Josh Triplett

Overall, this code looks sensible to me. Some comments on the patch below.

[ . . . ]

Quoted 3 lines

Thank you again for the careful and thorough review!!!

I will test these changes and send out an update.

And here is the updated RCU priority-boost patch.

Signed-off-by Paul E. McKenney <[email protected]>