Messages in this thread | | | Date | Wed, 16 Nov 2011 14:44:47 -0800 | From | "Paul E. McKenney" <> | Subject | Re: [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; }
| |