Message-ID: <20111116221545.GA2325@leaf>
Date:	Wed, 16 Nov 2011 14:15:45 -0800
From:	Josh Triplett <josh@...htriplett.org>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:	linux-kernel@...r.kernel.org, mingo@...e.hu, laijs@...fujitsu.com,
	dipankar@...ibm.com, akpm@...ux-foundation.org,
	mathieu.desnoyers@...ymtl.ca, niv@...ibm.com, tglx@...utronix.de,
	peterz@...radead.org, rostedt@...dmis.org, Valdis.Kletnieks@...edu,
	dhowells@...hat.com, eric.dumazet@...il.com, darren@...art.com,
	patches@...aro.org, "Paul E. McKenney" <paul.mckenney@...aro.org>
Subject: Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown
 capability

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@...aro.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@...aro.org>
> > > Signed-off-by: Paul E. McKenney <paulmck@...ux.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.

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

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

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

- 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