lkml.org 
[lkml]   [2011]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability
On Wed, Nov 16, 2011 at 02:58:56PM -0800, Josh Triplett wrote:
> On Wed, Nov 16, 2011 at 02:44:47PM -0800, Paul E. McKenney wrote:
> > On Wed, Nov 16, 2011 at 02:15:45PM -0800, Josh Triplett wrote:
> > > On Wed, Nov 16, 2011 at 12:32:26PM -0800, Paul E. McKenney wrote:
> > > > On Tue, Nov 15, 2011 at 01:46:15PM -0800, Josh Triplett wrote:
> > > > > On Tue, Nov 15, 2011 at 12:27:58PM -0800, Paul E. McKenney wrote:
> > > > > > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > >
> > > > > > Although it is easy to run rcutorture tests under KVM, there is currently
> > > > > > no nice way to run such a test for a fixed time period, collect all of
> > > > > > the rcutorture data, and then shut the system down cleanly. This commit
> > > > > > therefore adds an rcutorture module parameter named "shutdown_secs" that
> > > > > > specified the run duration in seconds, after which rcutorture terminates
> > > > > > the test and powers the system down. The default value for "shutdown_secs"
> > > > > > is zero, which disables shutdown.
> > > > > >
> > > > > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > >
> > > > > >From your recent post on this, I thought you found a solution through
> > > > > the init= parameter, which seems preferable.
> > > >
> > > > For some things, the init= parameter does work great. I do intend to
> > > > use it when collecting event-tracing and debugfs data, for example.
> > > >
> > > > However, there is still a need for RCU torture testing that will operate
> > > > correctly regardless of how userspace is set up. That, and there are
> > > > quite a few different kernel test setup, each with their own peculiar
> > > > capabilities and limitations. So what happened was that before people
> > > > suggested the init= approach, I implemented enough of the in-kernel
> > > > approach to appreciate how much it simplifies life for the common case of
> > > > "just torture-test RCU". As in I should have done this long ago.
> > >
> > > Seems like it would work just as easily to point init at a statically
> > > linked C program which just sleeps for a fixed time and then shuts down.
> > > However, given the special-purpose nature of rcutorture, I won't
> > > complain that strongly.
> >
> > I did consider a statically linked C program, but that can introduce the
> > need for cross-compilation into situations that do not otherwise need it.
>
> Wouldn't you need to cross-compile the kernel anyway in such situations?

Not necessarily, consider for example ABAT. (IBM-specific test setup
for those unfamiliar with it -- related to autotest.)

I suspect that the only way for you to be convinced is for you to write
a script that takes your preferred approach for injecting a test into
(say) a KVM instance.

Then compare that script to adding a few parameters to the boot line,
namely: "rcutorture.stat_interval=15 rcutorture.shutdown_secs=3600
rcutorture.rcutorture_runnable=1". ;-)

Using the parameters allows me to not care about the filesystem type, any
need to fsck, instruction set, the distro, and even the type of userspace.
Highly recommended!

> > > > > > +static int
> > > > > > +rcu_torture_shutdown(void *arg)
> > > > > > +{
> > > > > > + VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> > > > > > + while (ULONG_CMP_LT(jiffies, shutdown_time) &&
> > > > > > + !kthread_should_stop()) {
> > > > > > + if (verbose)
> > > > > > + printk(KERN_ALERT "%s" TORTURE_FLAG
> > > > > > + "rcu_torture_shutdown task: %lu "
> > > > > > + "jiffies remaining\n",
> > > > > > + torture_type, shutdown_time - jiffies);
> > > > > > + schedule_timeout_interruptible(HZ);
> > > > > > + }
> > > > >
> > > > > Any particular reason to wake up once a second here? If !verbose, this could just
> > > > > sleep until shutdown time. (And does the verbose output really help
> > > > > here, given printk timestamps?)
> > > >
> > > > It actually did help me find a bug where it was failing to shut down.
> > > > I could use different code paths, but that would defeat the debugging.
> > > >
> > > > So I increased the sleep time to 30 seconds. Fair enough?
> > >
> > > Well, now that you've debugged rcutorture's shutdown routine, would it
> > > suffice to have a printk when you actually go to shut down, without
> > > waking up for previous printks when not shutting down yet?
> > >
> > > (The poll time doesn't really matter, and sleeping for 30 seconds before
> > > checking the time means you might overshoot by up to 30 seconds. I'd
> > > like to avoid polling to begin with when you know exactly how long you
> > > need to sleep.)
> >
> > Indeed, good points! But please see below for what this function turns
> > into when taking that approach.
>
> See below for responses; that version seems like an improvement, though
> it could still improve further.
>
> > rcu_torture_shutdown(void *arg)
> > {
> > long delta;
> > unsigned long jiffies_snap;
> >
> > VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> > jiffies_snap = ACCESS_ONCE(jiffies);
>
> Why do you need to snapshot jiffies in this version but not in the
> version you originally posted?

Because in the original, the maximum error was one second, which was
not worth worrying about.

> > while (ULONG_CMP_LT(jiffies_snap, shutdown_time) &&
> > !kthread_should_stop()) {
> > delta = shutdown_time - jiffies_snap;
> > if (verbose)
> > printk(KERN_ALERT "%s" TORTURE_FLAG
> > "rcu_torture_shutdown task: %lu "
> > "jiffies remaining\n",
> > torture_type, delta);
>
> I suggested dropping this print entirely; under normal circumstances it
> should never print. It will only print if
> schedule_timeout_interruptible wakes up spuriously.

OK, I can qualify with a firsttime local variable.

> > schedule_timeout_interruptible(delta);
> > jiffies_snap = ACCESS_ONCE(jiffies);
> > }
>
> Any reason this entire loop body couldn't just become
> msleep_interruptible()?

Aha!!! Because then it won't break out of the loop if someone does
a rmmod of rcutorture. Which will cause the rmmod to hang until
the thing decides that it is time to shut down the system. Which
is why I need to do the sleep in smallish pieces -- I cannot sleep
longer than I would be comfortable delaying the rmmod.

Which is why I think I need to revert back to the old version that
did the schedule_timeout_interruptible(1).

> > if (ULONG_CMP_LT(jiffies_snap, shutdown_time)) {
> > VERBOSE_PRINTK_STRING("rcu_torture_shutdown task stopping");
> > return 0;
> > }
>
> Writing this as "if (kthread_should_stop())" seems clearer.

I don't have any real preference here, but as long as I need to
back out the earlier changes, I might as well make this one. ;-)

Thanx, Paul



\
 
 \ /
  Last update: 2011-11-17 00:47    [W:1.294 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site