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

> > > > --- a/kernel/rcutorture.c
> > > > +++ b/kernel/rcutorture.c
> > > > @@ -61,9 +61,10 @@ static int test_no_idle_hz; /* Test RCU's support for tickless idle CPUs. */
> > > > static int shuffle_interval = 3; /* Interval between shuffles (in sec)*/
> > > > static int stutter = 5; /* Start/stop testing interval (in sec) */
> > > > static int irqreader = 1; /* RCU readers from irq (timers). */
> > > > -static int fqs_duration = 0; /* Duration of bursts (us), 0 to disable. */
> > > > -static int fqs_holdoff = 0; /* Hold time within burst (us). */
> > > > +static int fqs_duration; /* Duration of bursts (us), 0 to disable. */
> > > > +static int fqs_holdoff; /* Hold time within burst (us). */
> > >
> > > Looks like these lines picked up unrelated whitespace changes in this
> > > commit.
> >
> > Turns out that my initial patch added another variable that I explicitly
> > initialized to zero. Of course, checkpatch yelled at me about this, so
> > I figured I should fix the other nearby occurrences of this while I was
> > at it. Doesn't really seem to me to be worth a separate patch, though.
>
> Ah, I missed the removal of the initializer. However, I don't see the
> harm in splitting out the trivial two-line patch, rather than folding it
> into an unrelated change which just happens to change lines nearby.

Ummm... Laziness on my part? ;-)

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

> > > > + if (ULONG_CMP_LT(jiffies, shutdown_time)) {
> > > > + VERBOSE_PRINTK_STRING("rcu_torture_shutdown task stopping");
> > > > + return 0;
> > > > + }
> > > > +
> > > > + /* OK, shut down the system. */
> > > > +
> > > > + VERBOSE_PRINTK_STRING("rcu_torture_shutdown task shutting down system");
> > > > + shutdown_task = NULL; /* Avoid self-kill deadlock. */
> > >
> > > Not that it matters much here, but won't this cause a leak?
> >
> > Only if we are shutting down. And the alternative is a deadlock
> > where this task invokes kthread_stop() on itself. ;-)
>
> Hence why I said it didn't matter much. :)

;-)

Thanx, Paul

------------------------------------------------------------------------

rcu_torture_shutdown(void *arg)
{
long delta;
unsigned long jiffies_snap;

VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
jiffies_snap = ACCESS_ONCE(jiffies);
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);
schedule_timeout_interruptible(delta);
jiffies_snap = ACCESS_ONCE(jiffies);
}
if (ULONG_CMP_LT(jiffies_snap, shutdown_time)) {
VERBOSE_PRINTK_STRING("rcu_torture_shutdown task stopping");
return 0;
}

/* OK, shut down the system. */

VERBOSE_PRINTK_STRING("rcu_torture_shutdown task shutting down system");
shutdown_task = NULL; /* Avoid self-kill deadlock. */
rcu_torture_cleanup(); /* Get the success/failure message. */
kernel_power_off(); /* Shut down the system. */
return 0;
}



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