Re: [RFC PATCH -rt 2/2] RCU priority boosting additions to rcutorture

On Thu, Jan 25, 2007 at 11:06:35AM -0800, Josh Triplett wrote:
Paul E. McKenney wrote:
On Thu, Jan 25, 2007 at 12:47:04AM -0800, Josh Triplett wrote:
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.

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

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.

I considered that for this initial round, but you only need to preempt
a single RCU reader to force the RCU booster to do something. ;-)

Paul E. McKenney wrote:
diff -urpNa -X dontdiff linux-2.6.20-rc4-rt1/kernel/rcutorture.c linux-2.6.20-rc4-rt1-rcubtorture/kernel/rcutorture.c
--- linux-2.6.20-rc4-rt1/kernel/rcutorture.c 2007-01-09 10:59:54.000000000 -0800
+++ linux-2.6.20-rc4-rt1-rcubtorture/kernel/rcutorture.c 2007-01-23 11:27:49.000000000 -0800

+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) &&
+ (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.

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.

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

OK.

+ } 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.

Even better, yes.

+ 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.

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

Sounds good to me! ;-)

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"
};

@@ -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? ;-)

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

I removed all the "= NULL" entries.

Thanks.

@@ -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. ;-)

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

I will run this through the mill and repost.

But with the new kernel parameter this time. ;-)

Thanx, Paul
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages