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

Paul E. McKenney wrote:
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.

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.

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

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

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

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

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

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

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

This looks like a bit of stray code.

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.

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

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

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

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

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

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

Likewise.

- Josh Triplett
-
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