| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 25 Jan 2007 00:47:04 -0800
From: Josh Triplett <josh@...nel.org>
To: paulmck@...ux.vnet.ibm.com
CC: linux-kernel@...r.kernel.org, mingo@...e.hu, tglx@...uxtronix.de,
dipankar@...ibm.com, tytso@...ibm.com, dvhltc@...ibm.com,
oleg@...sign.ru, twoerner.k@...il.com, billh@...ppy.monkey.org,
nielsen.esben@...glemail.com, corbet@....net
Subject: 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists