Message-ID: <45B86E88.6020209@kernel.org>
Date: Thu, 25 Jan 2007 00:47:04 -0800
From: Josh Triplett <josh@kernel.org>
User-Agent: Icedove 1.5.0.9 (X11/20061220)
MIME-Version: 1.0
To: paulmck@linux.vnet.ibm.com
CC: linux-kernel@vger.kernel.org, mingo@elte.hu, tglx@linuxtronix.de,
       dipankar@in.ibm.com, tytso@us.ibm.com, dvhltc@us.ibm.com,
       oleg@tv-sign.ru, twoerner.k@gmail.com, billh@gnuppy.monkey.org,
       nielsen.esben@googlemail.com, corbet@lwn.net
Subject: Re: [RFC PATCH -rt 2/2] RCU priority boosting additions to rcutorture
References: <20070125021101.GA23428@linux.vnet.ibm.com> <20070125021444.GA22540@linux.vnet.ibm.com> <20070125022338.GB22540@linux.vnet.ibm.com>
In-Reply-To: <20070125022338.GB22540@linux.vnet.ibm.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6565
Lines: 219

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/