| Message ID | 1321388885-11211-2-git-send-email-paulmck@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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. > --- 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. > @@ -1305,6 +1313,37 @@ static int rcutorture_booster_init(int cpu) > return 0; > } > > +/* > + * Cause the rcutorture test to "stutter", starting and stopping all > + * threads periodically. > + */ This comment looks like a copy-paste error. > +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?) > + 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? > + rcu_torture_cleanup(); /* Get the success/failure message. */ > + kernel_power_off(); /* Shut down the system. */ > + return 0; > +} Huh. I would have expected kernel_power_off to use noreturn, making the return 0 unnecessary here; however, apparently it doesn't. - Josh Triplett
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. I will therefore be taking both approaches. There will be at least one more patch pushing what is now script into rcutorture.c. > > --- 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. > > @@ -1305,6 +1313,37 @@ static int rcutorture_booster_init(int cpu) > > return 0; > > } > > > > +/* > > + * Cause the rcutorture test to "stutter", starting and stopping all > > + * threads periodically. > > + */ > > This comment looks like a copy-paste error. Or maybe a copy-paste stutter. ;-) Good eyes, fixed! > > +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? > > + 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. ;-) > > + rcu_torture_cleanup(); /* Get the success/failure message. */ > > + kernel_power_off(); /* Shut down the system. */ > > + return 0; > > +} > > Huh. I would have expected kernel_power_off to use noreturn, making the > return 0 unnecessary here; however, apparently it doesn't. Indeed, gcc yelled at me, so I added the "return 0". ;-) Thanx, Paul
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. > > > --- 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
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; }
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? > > > > > +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? > 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. > schedule_timeout_interruptible(delta); > jiffies_snap = ACCESS_ONCE(jiffies); > } Any reason this entire loop body couldn't just become msleep_interruptible()? > 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. - Josh Triplett
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
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: > > > 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.) Which already handles compiling a kernel for you; ABAT just doesn't make it as easy to compile userspace programs as it does for kernels. :) > 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. > 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". ;-) I actually think stat_interval makes perfect sense, as does runnable. > > > 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. > > > 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. > > > 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. - Josh Triplett
[Resending with attachment this time.] 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: > > > 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.) Which already handles compiling a kernel for you; ABAT just doesn't make it as easy to compile userspace programs as it does for kernels. :) > 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. > 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". ;-) I actually think stat_interval makes perfect sense, as does runnable. > > > 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. > > > 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. > > > 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. - Josh Triplett
On Wed, Nov 16, 2011 at 04:49:23PM -0800, Josh Triplett wrote: > [Resending with attachment this time.] > > 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: > > > > 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.) > > Which already handles compiling a kernel for you; ABAT just doesn't make > it as easy to compile userspace programs as it does for kernels. :) Exactly! ;-) Between ABAT, its intended replacement, KVM, and who knows what all else, the thought of having rcutorture control the test duration via system shutdown is becoming increasingly attractive... > > 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. However, the thought of improving boot speed by confining the kernel to a very simple initrd does sound attractive. 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... 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" 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. > > 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". ;-) > > I actually think stat_interval makes perfect sense, as does runnable. Indeed, from what I can see, it is hard to have module parameters without also having the corresponding kernel boot parameters when that module is built directly into the kernel. ;-) > > > > 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. > > > > 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. > > > > 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? Thanx, Paul
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
On Thu, Nov 17, 2011 at 03:57:33PM -0800, Josh Triplett wrote: > 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. Except that I need my test-analysis scripts to be able to easily distinguish between a shutdown that rcutorture was OK with and a "rogue" shutdown that terminated the test prematurely. > > 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. Faster to build, as well. ;-) However, I am not convinced that initrd is generally applicable. I have seen kernels that get annoyed if the userspace doesn't take action within a given period of time. Such a kernel would not like being handed an initrd sandbox. > > 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. Quite possibly. > > 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?) Right -- I was setting both to different values in order to make sure I could tell which was having effect. In this case, the sleep-shutdown deadline arrived soonest, so it won. The "noapic" seemed to be necessary in my initial attempts to test RCU under qemu -- perhaps not needed for kvm? > > 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? Yes, easier and less fragile, especially with respect to automatically analyzing the test results. > 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. OK, I missed the "volatile" on jiffies. Bad cscope day, I guess. > > > > > > 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.) Exactly. It should never happen, so if I am worried enough about what rcutorture is doing to specify "verbose" (which I rarely do), then I want to see it. > > > > > > 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); Color me confused. Here is msleep_interruptible(): unsigned long msleep_interruptible(unsigned int msecs) { unsigned long timeout = msecs_to_jiffies(msecs) + 1; while (timeout && !signal_pending(current)) timeout = schedule_timeout_interruptible(timeout); return jiffies_to_msecs(timeout); } And here is signal_pending(): static inline int signal_pending(struct task_struct *p) { return unlikely(test_tsk_thread_flag(p,TIF_SIGPENDING)); } And finally, here is kthread_stop(): int kthread_stop(struct task_struct *k) { struct kthread *kthread; int ret; trace_sched_kthread_stop(k); get_task_struct(k); kthread = to_kthread(k); barrier(); /* it might have exited */ if (k->vfork_done != NULL) { kthread->should_stop = 1; wake_up_process(k); wait_for_completion(&kthread->exited); } ret = k->exit_code; put_task_struct(k); trace_sched_kthread_stop_ret(ret); return ret; } I don't see how kthread_stop() is doing anything to kick msleep_interruptible() out of its loop. What am I missing? Thanx, Paul
diff --git a/Documentation/RCU/torture.txt b/Documentation/RCU/torture.txt index 783d6c1..af40929 100644 --- a/Documentation/RCU/torture.txt +++ b/Documentation/RCU/torture.txt @@ -66,6 +66,11 @@ shuffle_interval to a particular subset of the CPUs, defaults to 3 seconds. Used in conjunction with test_no_idle_hz. +shutdown_secs The number of seconds to run the test before terminating + the test and powering off the system. The default is + zero, which disables test termination and system shutdown. + This capability is useful for automated testing. + stat_interval The number of seconds between output of torture statistics (via printk()). Regardless of the interval, statistics are printed when the module is unloaded. diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c index df35228..41802be 100644 --- 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). */ static int fqs_stutter = 3; /* Wait time between bursts (s). */ +static int shutdown_secs; /* Shutdown time (s). <=0 for no shutdown. */ static int test_boost = 1; /* Test RCU prio boost: 0=no, 1=maybe, 2=yes. */ static int test_boost_interval = 7; /* Interval between boost tests, seconds. */ static int test_boost_duration = 4; /* Duration of each boost test, seconds. */ @@ -91,6 +92,8 @@ module_param(fqs_holdoff, int, 0444); MODULE_PARM_DESC(fqs_holdoff, "Holdoff time within fqs bursts (us)"); module_param(fqs_stutter, int, 0444); MODULE_PARM_DESC(fqs_stutter, "Wait time between fqs bursts (s)"); +module_param(shutdown_secs, int, 0444); +MODULE_PARM_DESC(shutdown_secs, "Shutdown time (s), zero to disable."); module_param(test_boost, int, 0444); MODULE_PARM_DESC(test_boost, "Test RCU prio boost: 0=no, 1=maybe, 2=yes."); module_param(test_boost_interval, int, 0444); @@ -119,6 +122,7 @@ static struct task_struct *shuffler_task; static struct task_struct *stutter_task; static struct task_struct *fqs_task; static struct task_struct *boost_tasks[NR_CPUS]; +static struct task_struct *shutdown_task; #define RCU_TORTURE_PIPE_LEN 10 @@ -167,6 +171,7 @@ int rcutorture_runnable = RCUTORTURE_RUNNABLE_INIT; #define rcu_can_boost() 0 #endif /* #else #if defined(CONFIG_RCU_BOOST) && !defined(CONFIG_HOTPLUG_CPU) */ +static unsigned long shutdown_time; /* jiffies to system shutdown. */ static unsigned long boost_starttime; /* jiffies of next boost test start. */ DEFINE_MUTEX(boost_mutex); /* protect setting boost_starttime */ /* and boost task create/destroy. */ @@ -182,6 +187,9 @@ static int fullstop = FULLSTOP_RMMOD; */ static DEFINE_MUTEX(fullstop_mutex); +/* Forward reference. */ +static void rcu_torture_cleanup(void); + /* * Detect and respond to a system shutdown. */ @@ -1250,12 +1258,12 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, char *tag) "shuffle_interval=%d stutter=%d irqreader=%d " "fqs_duration=%d fqs_holdoff=%d fqs_stutter=%d " "test_boost=%d/%d test_boost_interval=%d " - "test_boost_duration=%d\n", + "test_boost_duration=%d shutdown_secs=%d\n", torture_type, tag, nrealreaders, nfakewriters, stat_interval, verbose, test_no_idle_hz, shuffle_interval, stutter, irqreader, fqs_duration, fqs_holdoff, fqs_stutter, test_boost, cur_ops->can_boost, - test_boost_interval, test_boost_duration); + test_boost_interval, test_boost_duration, shutdown_secs); } static struct notifier_block rcutorture_shutdown_nb = { @@ -1305,6 +1313,37 @@ static int rcutorture_booster_init(int cpu) return 0; } +/* + * Cause the rcutorture test to "stutter", starting and stopping all + * threads periodically. + */ +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); + } + 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. */ + rcu_torture_cleanup(); /* Get the success/failure message. */ + kernel_power_off(); /* Shut down the system. */ + return 0; +} + static int rcutorture_cpu_notify(struct notifier_block *self, unsigned long action, void *hcpu) { @@ -1409,6 +1448,10 @@ rcu_torture_cleanup(void) for_each_possible_cpu(i) rcutorture_booster_cleanup(i); } + if (shutdown_task != NULL) { + VERBOSE_PRINTK_STRING("Stopping rcu_torture_shutdown task"); + kthread_stop(shutdown_task); + } /* Wait for all RCU callbacks to fire. */ @@ -1625,6 +1668,17 @@ rcu_torture_init(void) } } } + if (shutdown_secs > 0) { + shutdown_time = jiffies + shutdown_secs * HZ; + shutdown_task = kthread_run(rcu_torture_shutdown, NULL, + "rcu_torture_shutdown"); + if (IS_ERR(shutdown_task)) { + firsterr = PTR_ERR(shutdown_task); + VERBOSE_PRINTK_ERRSTRING("Failed to create shutdown"); + shutdown_task = NULL; + goto unwind; + } + } register_reboot_notifier(&rcutorture_shutdown_nb); rcutorture_record_test_transition(); mutex_unlock(&fullstop_mutex);