Message-ID: <20111117235733.GF1865@leaf>
Date:	Thu, 17 Nov 2011 15:57:33 -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 05:40:57PM -0800, Paul E. McKenney wrote:
> On Wed, Nov 16, 2011 at 04:49:23PM -0800, Josh Triplett wrote:
> > On Wed, Nov 16, 2011 at 03:43:58PM -0800, Paul E. McKenney wrote:
> > > 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.
> > 
> > Done and attached.
> 
> Cute trick!
> 
> The scripting has to also include getting the test duration into the .c
> file and building it, but that isn't too big of a deal.  Give or take
> cross-compilation of the sleep-shutdown.c program, that is...  :-/

:)

> However, this approach does cause the rcutorture success/failure message
> to be lost.  One possible way around this would be to put rcutorture.ko
> into your initrd, then make the program insmod it, sleep, rmmod it,
> and then shut down.  But unless I am missing something basic (which is
> quite possible), just doing the shutdown inside rcutorture is simpler
> at that point.

When you build rcutorture into the kernel, the module's exit function
will never get called at all.  If you want to see the final summary, you
might want to build in a mechanism for rcutorture to quiesce even when
built in, and then arrange to run that via a shutdown notifier.  That
seems like the right way around: you shut down the system, and the
built-in rcutorture notices and gives a final summary.

Alternatively, similar to your addition of rcutorture.stat_interval to
periodically write out a summary, you might consider having rcutorture
periodically quiesce and write out a PASS/FAIL.

> However, the thought of improving boot speed by confining the kernel to
> a very simple initrd does sound attractive.

Definitely.  I frequently use an initrd rather than creating an entire
filesystem, and the result runs very quickly.  It also proves easier to
build than a block device.

> Interesting behavior.  I forgot that the bzImage that I had lying around
> already had an initrd built into it.  The kernel seemed to start up on
> the built-in initrd, complete with serial output.  Then powered off after
> about 30 seconds (I did s/3600/30/ in your sleep-shutdown.c), rather than
> the 120 I had specified to rcutorture (preventing any success/failure
> message from rcutorture as well).  Two initrds executing in parallel?
> Hmmm...

Huh.  Odd.  But no, initramfses don't execute in parallel, or in series
for that matter.  The kernel will extract its built-in initramfs to the
"rootfs" tmpfs filesystem, then extract any externally-supplied
initramfs on top of that, and then run /init in the rootfs filesystem.
So, the /init from sleep-shutdown would overwrite any /init from your
built-in initramfs, and most likely nothing else from your built-in
initramfs had any effect at all.

> FWIW, I used the following command:
> 
> kvm -kernel linux-2.6/arch/x86/boot/bzImage -initrd /tmp/sleep-shutdown/sleep-shutdown -serial file:/home/kvm/linux-2.6/console.log -nographic -smp 2 -m 512 -append "noapic selinux=0 console=ttyS0 initcall_debug debug rcutorture.stat_interval=15 rcutorture.shutdown_secs=120 rcutorture.rcutorture_runnable=1"

Looks reasonable, other than that you don't need
rcutorture.shutdown_secs if you use sleep-shutdown.  Also, as far as I
know you shouldn't need noapic with KVM; if you do then you should
report a KVM bug.  (Or does that represent a scenario or code path you
wanted to test RCU under?)

> Thoughts?  (Other than that I should re-build the kernel with
> CONFIG_INITRAMFS_SOURCE="", which I will try.)
> 
> Just so you know, my next step is to make rcutorture orchestrate the
> CPU-hotplug operations, if rcutorture is told to do so and if the kernel
> is configured to support this.

And that really seems easier than just running a simple script from an
initrd?

On Wed, Nov 16, 2011 at 05:40:57PM -0800, Paul E. McKenney wrote:
> On Wed, Nov 16, 2011 at 04:49:23PM -0800, Josh Triplett wrote:
> > On Wed, Nov 16, 2011 at 03:43:58PM -0800, Paul E. McKenney wrote:
> > > 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:
> > > > > 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.
> > 
> > The original shouldn't have an error either.  If something incorrectly
> > caches jiffies, either version would sleep forever, not just for an
> > extra second.
> 
> If it cached it from one iteration of the loop to the next, agreed.
> But the new version of the code introduces other possible ways for the
> compiler to confuse the issue.

At least in theory, the compiler can't cache the value of jiffies at
all, since jiffies uses "volatile".  The CPU potentially could, but we
don't normally expect CPUs to hold onto old cached values indefinitely;
we tend to count on updates to eventually propagate.

> > > > > 	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.
> > 
> > Oh, i see; it does print the very first time through.  In that case, you
> > could move the print out of the loop entirely, rather than using a
> > "first time" flag.
> 
> I could, but that would prevent me from seeing cases where there are
> multiple passes through the loop.

...which should never happen unless something signals that thread,
right?  (And normally, that signal will occur as part of kthread_stop,
which would terminate the loop as well.)

> > > > > 		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).
> > 
> > Does kthread_stop not interrupt an interruptible kthread?  As far as I
> > can tell, rmmod of rcutorture currently finishes immediately, rather
> > than after all the one-second sleeps finish, which suggests that it
> > wakes up the threads in question.
> 
> OK, it might well.  But even if kthread_stop() does interrupt an
> interruptible kthread, I still need to avoid msleep_interruptible(),
> right?

I don't see any reason you need to avoid it.  As far as I can tell,
msleep_interruptible should do *exactly* what you want, including
allowing interruptions by kthread_stop.  You just need something like
this:

unsigned int timeout = ...;
while (timeout && !kthread_should_stop())
    timeout = msleep_interruptible(timeout);

- 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