Preview of additional RCU changes for 3.3
Subsystems (6)
cover Preview of additional RCU changes for 3.3
Hello!
This patchset is in addition to the set previously posted (see
https://lkml.org/lkml/2011/11/2/363). This new set permits the reworked RCU_FAST_NO_HZ code to be used from TREE_PREEMPT_RCU, allows rcutorture to shut down the system after a fixed time period (useful for KVM-based testing), adds an API to replace the "->pid == 0" discussed in response to the earlier patch set, and applies this API where applicable. The patches are as follows:
1. Update the RCU_FAST_NO_HZ config definition to allow it to
be used in TREE_PREEMPT_RCU builds, since that is now legal.
2. Add a module parameter that permit rcutorture to shut down the
system cleanly after the test has run for the specified time.
3. Add a module parameter to allow rcutorture start running
as soon as it is initialized.
4. Add is_idle_task() API to replace open-coded tests of ->pid
against zero.
5-9. Apply the new is_idle_task() API where it makes sense.
For a testing-only version of this patchset from git, please see the following subject-to-rebase branch:
git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git rcu/dev
Thanx, Paul
------------------------------------------------------------------------
b/Documentation/RCU/torture.txt | 5 +++ b/arch/sparc/kernel/setup_32.c | 2 - b/arch/tile/mm/fault.c | 4 +- b/include/linux/sched.h | 1 b/init/Kconfig | 10 +++--- b/kernel/debug/kdb/kdb_support.c | 2 - b/kernel/events/core.c | 2 - b/kernel/rcutiny.c | 4 +- b/kernel/rcutorture.c | 62 ++++++++++++++++++++++++++++++++++++--- b/kernel/rcutree.c | 4 +- b/kernel/sched.c | 9 +++++ kernel/rcutorture.c | 2 + 12 files changed, 89 insertions(+), 18 deletions(-)
PATCH 1/9 rcu: Permit RCU_FAST_NO_HZ to be used by TREE_PREEMPT_RCU
From: Paul E. McKenney <[email protected]>
The new implementation of RCU_FAST_NO_HZ is compatible with preemptible RCU, so this commit removes the Kconfig restriction that previously prohibited this.
--- init/Kconfig | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
init/Kconfig +5 -5
@@ -469,14 +469,14 @@ config RCU_FANOUT_EXACT config RCU_FAST_NO_HZ bool "Accelerate last non-dyntick-idle CPU's grace periods" - depends on TREE_RCU && NO_HZ && SMP + depends on NO_HZ && SMP default n help This option causes RCU to attempt to accelerate grace periods - in order to allow the final CPU to enter dynticks-idle state - more quickly. On the other hand, this option increases the - overhead of the dynticks-idle checking, particularly on systems - with large numbers of CPUs. + in order to allow CPUs to enter dynticks-idle state more + quickly. On the other hand, this option increases the overhead + of the dynticks-idle checking, particularly on systems with + large numbers of CPUs. Say Y if energy efficiency is critically important, particularly if you have relatively few CPUs.
Signature · 1 line
1.7.3.2
PATCH 2/9 rcu: Add rcutorture system-shutdown capability
In reply to Paul E. McKenneyFrom: Paul E. McKenney <[email protected]>
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.
--- Documentation/RCU/torture.txt | 5 +++ kernel/rcutorture.c | 62 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 63 insertions(+), 4 deletions(-)
Documentation/RCU/torture.txt +5 -0
@@ -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.
kernel/rcutorture.c +58 -4
121 lines not on this page · raw message
Signature · 1 line
1.7.3.2
Josh Triplett
maintainerRe: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability
Quoted 10 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
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.
Quoted 4 lines
Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>From your recent post on this, I thought you found a solution through
the init= parameter, which seems preferable.
kernel/rcutorture.c static int test_no_idle_hz; /* Test RCU's support for tickless idle CPUs. */ · line 65
Quoted 10 lines
kernel/rcutorture.c +2
-2@@ -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.
kernel/rcutorture.c static int rcutorture_booster_init(int cpu) · line 1319
Quoted 8 lines
kernel/rcutorture.c +4
-0@@ -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.
kernel/rcutorture.c static int rcutorture_booster_init(int cpu) · line 1332
Quoted 13 lines
kernel/rcutorture.c +13
-0+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?)
kernel/rcutorture.c static int rcutorture_booster_init(int cpu) · line 1341
Quoted 9 lines
kernel/rcutorture.c +9
-0+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?
kernel/rcutorture.c static int rcutorture_booster_init(int cpu) · line 1345
Quoted 4 lines
kernel/rcutorture.c +4
-0+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
Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability
Quoted 11 lines · Josh Triplett
Quoted 10 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
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.
Quoted 5 lines
Quoted 4 lines
Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>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.
Quoted 5 lines
Quoted 5 lines
kernel/rcutorture.c +0
-0@@ -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) */
Quoted 8 lines
kernel/rcutorture.c static int test_no_idle_hz; /* Test RCU's support for tickless idle CPUs. */ · line 65
Quoted 5 lines
kernel/rcutorture.c +2
-2static 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.
Quoted 10 lines
kernel/rcutorture.c static int rcutorture_booster_init(int cpu) · line 1319
Quoted 8 lines
kernel/rcutorture.c +4
-0@@ -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!
Quoted 9 lines
Quoted 9 lines
kernel/rcutorture.c +9
-0+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 "
Quoted 8 lines
kernel/rcutorture.c static int rcutorture_booster_init(int cpu) · line 1332
Quoted 4 lines
kernel/rcutorture.c +4
-0+"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?
Quoted 11 lines
kernel/rcutorture.c static int rcutorture_booster_init(int cpu) · line 1341
Quoted 9 lines
kernel/rcutorture.c +9
-0+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. ;-)
Quoted 7 lines
kernel/rcutorture.c static int rcutorture_booster_init(int cpu) · line 1345
Quoted 4 lines
kernel/rcutorture.c +4
-0+rcu_torture_cleanup ();/* Get the success/failure message. */ +kernel_power_off ();/* Shut down the system. */ +return 0 ; +}Huh. I would have expected
kernel_power_offto 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
Josh Triplett
maintainerRe: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability
Quoted 21 lines · Paul E. McKenney
Quoted 11 lines · Josh Triplett
Quoted 10 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
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.Quoted 5 lines
Quoted 4 lines
Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>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.
Quoted 7 lines
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.
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
kernel/rcutorture.c +0
-0@@ -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) */ Quoted 8 lines
kernel/rcutorture.c static int test_no_idle_hz; /* Test RCU's support for tickless idle CPUs. */ · line 65
Quoted 5 lines
kernel/rcutorture.c +2
-2static 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.
Quoted 4 lines
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.
Quoted 18 lines
Quoted 9 lines
Quoted 9 lines
kernel/rcutorture.c +9
-0+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 "Quoted 8 lines
kernel/rcutorture.c static int rcutorture_booster_init(int cpu) · line 1332
Quoted 4 lines
kernel/rcutorture.c +4
-0+"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?)
Quoted 4 lines
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.)
Quoted 12 lines
Quoted 11 lines
kernel/rcutorture.c static int rcutorture_booster_init(int cpu) · line 1341
Quoted 9 lines
kernel/rcutorture.c +9
-0+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?
Quoted 2 lines
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
Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability
Quoted 30 lines · Josh Triplett
Quoted 21 lines · Paul E. McKenney
Quoted 11 lines · Josh Triplett
Quoted 10 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
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.Quoted 5 lines
Quoted 4 lines
Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>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.
Quoted 7 lines
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.
Quoted 4 lines
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.
Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
kernel/rcutorture.c static int test_no_idle_hz; /* Test RCU's support for tickless idle CPUs. */ · line 65
Quoted 10 lines
kernel/rcutorture.c +2
-2@@ -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.
Quoted 8 lines
Quoted 4 lines
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? ;-)
Quoted 23 lines
Quoted 18 lines
Quoted 9 lines
Quoted 9 lines
kernel/rcutorture.c +9
-0+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 "Quoted 8 lines
kernel/rcutorture.c static int rcutorture_booster_init(int cpu) · line 1332
Quoted 4 lines
kernel/rcutorture.c +4
-0+"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?)
Quoted 4 lines
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?
Quoted 8 lines
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.
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
kernel/rcutorture.c static int rcutorture_booster_init(int cpu) · line 1341
Quoted 9 lines
kernel/rcutorture.c +9
-0+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?
Quoted 4 lines
Quoted 2 lines
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;
}Josh Triplett
maintainerRe: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability
Quoted 31 lines · Paul E. McKenney
Quoted 30 lines · Josh Triplett
Quoted 28 lines · Paul E. McKenney
Quoted 11 lines · Josh Triplett
Quoted 10 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
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.Quoted 5 lines
Quoted 4 lines
Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>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.
Quoted 7 lines
Quoted 4 lines
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?
Quoted 27 lines
Quoted 27 lines
Quoted 18 lines
Quoted 9 lines
Quoted 9 lines
kernel/rcutorture.c +9
-0+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 "Quoted 8 lines
kernel/rcutorture.c static int rcutorture_booster_init(int cpu) · line 1332
Quoted 4 lines
kernel/rcutorture.c +4
-0+"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?)
Quoted 4 lines
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?
Quoted 7 lines
Quoted 4 lines
(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.
Quoted 7 lines
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?
Quoted 8 lines
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.
Quoted 3 lines
schedule_timeout_interruptible(delta); jiffies_snap = ACCESS_ONCE(jiffies); }
Any reason this entire loop body couldn't just become msleep_interruptible()?
Quoted 4 lines
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
Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability
Quoted 37 lines · Josh Triplett
Quoted 36 lines · Paul E. McKenney
Quoted 34 lines · Josh Triplett
Quoted 21 lines · Paul E. McKenney
Quoted 11 lines · Josh Triplett
Quoted 10 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
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.Quoted 5 lines
Quoted 4 lines
Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>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.
Quoted 7 lines
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.
Quoted 4 lines
Quoted 2 lines
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!
Quoted 43 lines
Quoted 27 lines
Quoted 27 lines
Quoted 18 lines
Quoted 9 lines
Quoted 9 lines
kernel/rcutorture.c +9
-0+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 "Quoted 8 lines
kernel/rcutorture.c static int rcutorture_booster_init(int cpu) · line 1332
Quoted 4 lines
kernel/rcutorture.c +4
-0+"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?)
Quoted 4 lines
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?
Quoted 7 lines
Quoted 4 lines
(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.
Quoted 5 lines
rcu_torture_shutdown(void *arg){ long delta; unsigned long jiffies_snap;
Quoted 5 lines
Quoted 2 lines
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.
Quoted 9 lines
Quoted 8 lines
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);
Quoted 3 lines
I suggested dropping this print entirely; under normal circumstances it should never print. It will only print if
schedule_timeout_interruptiblewakes up spuriously.
OK, I can qualify with a firsttime local variable.
Quoted 6 lines
Quoted 3 lines
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).
Quoted 6 lines
Quoted 4 lines
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
Josh Triplett
maintainerRe: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability
Quoted 38 lines · Paul E. McKenney
Quoted 37 lines · Josh Triplett
Quoted 36 lines · Paul E. McKenney
Quoted 34 lines · Josh Triplett
Quoted 21 lines · Paul E. McKenney
Quoted 11 lines · Josh Triplett
Quoted 10 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
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.Quoted 5 lines
Quoted 4 lines
Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>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.
Quoted 7 lines
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.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
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. :)
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
rcu_torture_shutdown(void *arg){ long delta; unsigned long jiffies_snap;
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
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);
Quoted 5 lines
Quoted 3 lines
I suggested dropping this print entirely; under normal circumstances it should never print. It will only print if
schedule_timeout_interruptiblewakes 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.
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
schedule_timeout_interruptible(delta); jiffies_snap = ACCESS_ONCE(jiffies); }Any reason this entire loop body couldn't just become
msleep_interruptible()?
Quoted 8 lines
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
Josh Triplett
maintainerRe: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability
In reply to Paul E. McKenney[Resending with attachment this time.]
Quoted 38 lines · Paul E. McKenney
Quoted 37 lines · Josh Triplett
Quoted 36 lines · Paul E. McKenney
Quoted 34 lines · Josh Triplett
Quoted 21 lines · Paul E. McKenney
Quoted 11 lines · Josh Triplett
Quoted 10 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
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.Quoted 5 lines
Quoted 4 lines
Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>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.
Quoted 7 lines
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.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
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. :)
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
rcu_torture_shutdown(void *arg){ long delta; unsigned long jiffies_snap;
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
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);
Quoted 5 lines
Quoted 3 lines
I suggested dropping this print entirely; under normal circumstances it should never print. It will only print if
schedule_timeout_interruptiblewakes 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.
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
schedule_timeout_interruptible(delta); jiffies_snap = ACCESS_ONCE(jiffies); }Any reason this entire loop body couldn't just become
msleep_interruptible()?
Quoted 8 lines
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
Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability
Quoted 44 lines · Josh Triplett
[Resending with attachment this time.]
Quoted 41 lines · Paul E. McKenney
Quoted 40 lines · Josh Triplett
Quoted 38 lines · Paul E. McKenney
Quoted 30 lines · Josh Triplett
Quoted 21 lines · Paul E. McKenney
Quoted 11 lines · Josh Triplett
Quoted 10 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
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.Quoted 5 lines
Quoted 4 lines
Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>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.
Quoted 7 lines
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.
Quoted 4 lines
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.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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...
Quoted 5 lines
Quoted 3 lines
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.
Quoted 5 lines
Quoted 3 lines
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_intervalmakes 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. ;-)
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 7 lines
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?
Quoted 6 lines
Quoted 2 lines
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.
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
Quoted 8 lines
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_interruptiblewakes up spuriously.
Quoted 5 lines
Quoted 1 line
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.
Quoted 13 lines
Quoted 13 lines
Quoted 6 lines
Quoted 3 lines
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.
Quoted 7 lines
Quoted 2 lines
Which is why I think I need to revert back to the old version that did the
schedule_timeout_interruptible(1).Does
kthread_stopnot 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
Josh Triplett
maintainerRe: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability
Quoted 6 lines · Paul E. McKenney
Quoted 5 lines · Josh Triplett
Quoted 3 lines · Paul E. McKenney
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.
Quoted 7 lines
Quoted 1 line
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.cprogram, that is... :-/
:)
Quoted 6 lines
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.
Quoted 2 lines
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.
Quoted 7 lines
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.
Quoted 3 lines
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=ttyS0initcall_debugdebug 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?)
Quoted 6 lines
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?
Quoted 18 lines · Paul E. McKenney
Quoted 17 lines · Josh Triplett
Quoted 15 lines · Paul E. McKenney
Quoted 11 lines · Josh Triplett
Quoted 7 lines · Paul E. McKenney
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
Quoted 8 lines
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_interruptiblewakes up spuriously.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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.)
Quoted 16 lines
Quoted 16 lines
Quoted 15 lines
Quoted 6 lines
Quoted 3 lines
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).
Quoted 8 lines
Quoted 4 lines
Does
kthread_stopnot 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 avoidmsleep_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
Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability
Quoted 31 lines · Josh Triplett
Quoted 6 lines · Paul E. McKenney
Quoted 5 lines · Josh Triplett
Quoted 3 lines · Paul E. McKenney
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.
Quoted 7 lines
Quoted 1 line
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.cprogram, that is... :-/:)
Quoted 6 lines
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.
Quoted 3 lines
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 7 lines
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...
Quoted 7 lines
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.
Quoted 9 lines
Quoted 3 lines
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=ttyS0initcall_debugdebug 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?
Quoted 9 lines
Quoted 6 lines
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.
Quoted 23 lines · Paul E. McKenney
Quoted 22 lines · Josh Triplett
Quoted 20 lines · Paul E. McKenney
Quoted 7 lines · Josh Triplett
Quoted 6 lines · Paul E. McKenney
Quoted 5 lines
rcu_torture_shutdown(void *arg){ long delta; unsigned long jiffies_snap;Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 19 lines
Quoted 19 lines
Quoted 18 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
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);Quoted 5 lines
Quoted 3 lines
I suggested dropping this print entirely; under normal circumstances it should never print. It will only print if
schedule_timeout_interruptiblewakes 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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 25 lines
Quoted 16 lines
Quoted 16 lines
Quoted 15 lines
Quoted 6 lines
Quoted 3 lines
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).Quoted 8 lines
Quoted 4 lines
Does
kthread_stopnot 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 avoidmsleep_interruptible(), right?
Quoted 8 lines
I don't see any reason you need to avoid it. As far as I can tell,
msleep_interruptibleshould do *exactly* what you want, including allowing interruptions bykthread_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
PATCH 3/9 rcu: Control rcutorture startup from kernel boot parameters
In reply to Paul E. McKenneyFrom: Paul E. McKenney <[email protected]>
Currently, if rcutorture is built into the kernel, it must be manually started or started from an init script. This is inconvenient for automated KVM testing, where it is good to be able to fully control rcutorture execution from the kernel parameters. This patch therefore adds a module parameter named "rcutorture_runnable" that defaults to zero ("don't start automatically"), but which can be set to one to cause rcutorture to start up immediately during boot.
--- kernel/rcutorture.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
kernel/rcutorture.c +2 -0
@@ -164,6 +164,8 @@ static int stutter_pause_test;#define RCUTORTURE_RUNNABLE_INIT 0#endif int rcutorture_runnable =RCUTORTURE_RUNNABLE_INIT ; +module_param (rcutorture_runnable, int,0444 ); +MODULE_PARM_DESC (rcutorture_runnable,"Start rcutorture at boot" );#if defined(CONFIG_RCU_BOOST ) && !defined(CONFIG_HOTPLUG_CPU )#define rcu_can_boost () 1
Signature · 1 line
1.7.3.2
Josh Triplett
maintainerRe: [PATCH tip/core/rcu 3/9] rcu: Control rcutorture startup from kernel boot parameters
Quoted 21 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
Currently, if rcutorture is built into the kernel, it must be manually started or started from an init script. This is inconvenient for automated KVM testing, where it is good to be able to fully control rcutorture execution from the kernel parameters. This patch therefore adds a module parameter named "
rcutorture_runnable" that defaults to zero ("don't start automatically"), but which can be set to one to cause rcutorture to start up immediately during boot.Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>--- kernel/rcutorture.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)kernel/rcutorture.c +0
-0
kernel/rcutorture.c static int stutter_pause_test; · line 168
Quoted 5 lines
kernel/rcutorture.c +2
-0@@ -164,6 +164,8 @@ static int stutter_pause_test;#define RCUTORTURE_RUNNABLE_INIT 0#endif int rcutorture_runnable =RCUTORTURE_RUNNABLE_INIT ; +module_param (rcutorture_runnable, int,0444 ); +MODULE_PARM_DESC (rcutorture_runnable,"Start rcutorture at boot" );
Perhaps this should become a bool rather than an int, so that the kernel would recognize various variations on the parameter value, such as "on" or "true".
- Josh Triplett
Re: [PATCH tip/core/rcu 3/9] rcu: Control rcutorture startup from kernel boot parameters
Quoted 17 lines · Josh Triplett
Quoted 16 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
Currently, if rcutorture is built into the kernel, it must be manually started or started from an init script. This is inconvenient for automated KVM testing, where it is good to be able to fully control rcutorture execution from the kernel parameters. This patch therefore adds a module parameter named "
rcutorture_runnable" that defaults to zero ("don't start automatically"), but which can be set to one to cause rcutorture to start up immediately during boot.Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>--- kernel/rcutorture.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
Quoted 14 lines
kernel/rcutorture.c static int stutter_pause_test; · line 168
Quoted 10 lines
kernel/rcutorture.c +2
-0@@ -164,6 +164,8 @@ static int stutter_pause_test;#define RCUTORTURE_RUNNABLE_INIT 0#endif int rcutorture_runnable =RCUTORTURE_RUNNABLE_INIT ; +module_param (rcutorture_runnable, int,0444 ); +MODULE_PARM_DESC (rcutorture_runnable,"Start rcutorture at boot" );Perhaps this should become a bool rather than an int, so that the kernel would recognize various variations on the parameter value, such as "on" or "true".
I had a funny feeling that I would be needing other values to do things like say at what phase of boot the test was to start.
But if no need for this sort of function appears in the next while, then switching to bool would indeed make a lot of sense.
Thanx, Paul
Josh Triplett
maintainerRe: [PATCH tip/core/rcu 3/9] rcu: Control rcutorture startup from kernel boot parameters
Quoted 33 lines · Paul E. McKenney
Quoted 17 lines · Josh Triplett
Quoted 16 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
Currently, if rcutorture is built into the kernel, it must be manually started or started from an init script. This is inconvenient for automated KVM testing, where it is good to be able to fully control rcutorture execution from the kernel parameters. This patch therefore adds a module parameter named "
rcutorture_runnable" that defaults to zero ("don't start automatically"), but which can be set to one to cause rcutorture to start up immediately during boot.Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>--- kernel/rcutorture.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)Quoted 14 lines
kernel/rcutorture.c static int stutter_pause_test; · line 168
Quoted 10 lines
kernel/rcutorture.c +2
-0@@ -164,6 +164,8 @@ static int stutter_pause_test;#define RCUTORTURE_RUNNABLE_INIT 0#endif int rcutorture_runnable =RCUTORTURE_RUNNABLE_INIT ; +module_param (rcutorture_runnable, int,0444 ); +MODULE_PARM_DESC (rcutorture_runnable,"Start rcutorture at boot" );Perhaps this should become a bool rather than an int, so that the kernel would recognize various variations on the parameter value, such as "on" or "true".
Quoted 5 lines
I had a funny feeling that I would be needing other values to do things like say at what phase of boot the test was to start.
But if no need for this sort of function appears in the next while, then switching to bool would indeed make a lot of sense.
Ah, fair enough.
- Josh Triplett
PATCH 4/9 sched: add is_idle_task() to handle invalidated uses of idle_cpu()
In reply to Paul E. McKenneyFrom: Paul E. McKenney <[email protected]>
Commit 908a3283 (Fix idle_cpu()) invalidated some uses of idle_cpu(), which used to say whether or not the CPU was running the idle task, but now instead says whether or not the CPU is running the idle task in the absence of pending wakeups. Although this new implementation gives a better answer to the question "is this CPU idle?", it also invalidates other uses that were made of idle_cpu().
This commit therefore introduces a new is_idle_task() API member that determines whether or not the specified task is one of the idle tasks, allowing open-coded "->pid == 0" sequences to be replaced by something more meaningful.
--- include/linux/sched.h | 1 + kernel/sched.c | 9 +++++++++ 2 files changed, 10 insertions(+), 0 deletions(-)
include/linux/sched.h +1 -0
@@ -2071,6 +2071,7 @@ extern int sched_setscheduler(struct task_struct *, int,extern int sched_setscheduler_nocheck (struct task_struct *,int ,const struct sched_param *);extern struct task_struct *idle_task (int cpu); +extern int is_idle_task (struct task_struct *p);extern struct task_struct *curr_task (int cpu);extern void set_curr_task (int cpu,struct task_struct *p);
kernel/sched.c +9 -0
@@ -5185,6 +5185,15 @@ struct task_struct *idle_task(int cpu) }/** +* is_idle_task - is the specified task an idle task? +* @tsk: the task in question. +*/ +int is_idle_task (struct task_struct *p) +{ +return p->pid ==0 ; +} + +/** * find_process_by_pid - find a process with a matching PID value. * @pid: the pid in question. */
Signature · 1 line
1.7.3.2
Josh Triplett
maintainerRe: [PATCH tip/core/rcu 4/9] sched: add is_idle_task() to handle invalidated uses of idle_cpu()
Quoted 28 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
Commit
908a3283(Fixidle_cpu()) invalidated some uses ofidle_cpu(), which used to say whether or not the CPU was running the idle task, but now instead says whether or not the CPU is running the idle task in the absence of pending wakeups. Although this new implementation gives a better answer to the question "is this CPU idle?", it also invalidates other uses that were made ofidle_cpu().This commit therefore introduces a new
is_idle_task()API member that determines whether or not the specified task is one of the idle tasks, allowing open-coded "->pid == 0" sequences to be replaced by something more meaningful.Suggested-by Josh Triplett <[email protected]>Suggested-by Peter Zijlstra <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>--- include/linux/sched.h | 1 + kernel/sched.c | 9 +++++++++ 2 files changed, 10 insertions(+), 0 deletions(-)include/linux/sched.h +0
-0
include/linux/sched.h extern int sched_setscheduler(struct task_struct *, int, · line 2074
Quoted 4 lines
include/linux/sched.h +1
-0@@ -2071,6 +2071,7 @@ extern int sched_setscheduler(struct task_struct *, int,extern int sched_setscheduler_nocheck (struct task_struct *,int ,const struct sched_param *);extern struct task_struct *idle_task (int cpu); +extern int is_idle_task (struct task_struct *p);
The kernel has a bool type; please use it here rather than int, to make the return value more obvious.
Also, any particular reason not to make this a static inline?
- Josh Triplett
Re: [PATCH tip/core/rcu 4/9] sched: add is_idle_task() to handle invalidated uses of idle_cpu()
Quoted 37 lines · Josh Triplett
Quoted 28 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
Commit
908a3283(Fixidle_cpu()) invalidated some uses ofidle_cpu(), which used to say whether or not the CPU was running the idle task, but now instead says whether or not the CPU is running the idle task in the absence of pending wakeups. Although this new implementation gives a better answer to the question "is this CPU idle?", it also invalidates other uses that were made ofidle_cpu().This commit therefore introduces a new
is_idle_task()API member that determines whether or not the specified task is one of the idle tasks, allowing open-coded "->pid == 0" sequences to be replaced by something more meaningful.Suggested-by Josh Triplett <[email protected]>Suggested-by Peter Zijlstra <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>--- include/linux/sched.h | 1 + kernel/sched.c | 9 +++++++++ 2 files changed, 10 insertions(+), 0 deletions(-)include/linux/sched.h +0
-0include/linux/sched.h extern int sched_setscheduler(struct task_struct *, int, · line 2074
Quoted 4 lines
include/linux/sched.h +1
-0@@ -2071,6 +2071,7 @@ extern int sched_setscheduler(struct task_struct *, int,extern int sched_setscheduler_nocheck (struct task_struct *,int ,const struct sched_param *);extern struct task_struct *idle_task (int cpu); +extern int is_idle_task (struct task_struct *p);The kernel has a bool type; please use it here rather than int, to make the return value more obvious.
Quoted 1 line
Also, any particular reason not to make this a static inline?
That does work for all the uses thus far, so made both suggested changes.
Thanx, Paul
PATCH 5/9 rcu: Make RCU use the new is_idle_task() API
In reply to Paul E. McKenneyFrom: Paul E. McKenney <[email protected]>
Change from direct comparison of ->pid with zero to is_idle_task().
--- kernel/rcutiny.c | 4 ++-- kernel/rcutree.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
kernel/rcutiny.c +2 -2
@@ -64,7 +64,7 @@ static void rcu_idle_enter_common(long long oldval)return ; }RCU_TRACE (trace_rcu_dyntick ("Start" , oldval, rcu_dynticks_nesting)); -if (current->pid !=0 ) { +if (!is_idle_task (current)) {struct task_struct *idle =idle_task (smp_processor_id ()); RCU_TRACE(trace_rcu_dyntick("Error on entry: not idle task",
@@ -118,7 +118,7 @@ static void rcu_idle_exit_common(long long oldval)return ; }RCU_TRACE (trace_rcu_dyntick ("End" , oldval, rcu_dynticks_nesting)); -if (current->pid !=0 ) { +if (!is_idle_task (current)) {struct task_struct *idle =idle_task (smp_processor_id ()); RCU_TRACE(trace_rcu_dyntick("Error on exit: not idle task",
kernel/rcutree.c +2 -2
@@ -355,7 +355,7 @@ static void rcu_idle_enter_common(struct rcu_dynticks *rdtp, long long oldval)return ; }trace_rcu_dyntick ("Start" , oldval, rdtp->dynticks_nesting); -if (current->pid !=0 ) { +if (!is_idle_task (current)) {struct task_struct *idle =idle_task (smp_processor_id ()); trace_rcu_dyntick("Error on entry: not idle task",
@@ -449,7 +449,7 @@ static void rcu_idle_exit_common(struct rcu_dynticks *rdtp, long long oldval)smp_mb__after_atomic_inc ();/* See above. */ WARN_ON_ONCE (!(atomic_read (&rdtp->dynticks) &0x1 ));trace_rcu_dyntick ("End" , oldval, rdtp->dynticks_nesting); -if (current->pid !=0 ) { +if (!is_idle_task (current)) {struct task_struct *idle =idle_task (smp_processor_id ()); trace_rcu_dyntick("Error on exit: not idle task",
Signature · 1 line
1.7.3.2
Re: [PATCH tip/core/rcu 5/9] rcu: Make RCU use the new is_idle_task() API
Quoted 7 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
Change from direct comparison of ->pid with zero to
is_idle_task().Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>---
Quoted 51 lines
kernel/rcutiny.c | 4 ++-- kernel/rcutree.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)kernel/rcutiny.c +2
-2@@ -64,7 +64,7 @@ static void rcu_idle_enter_common(long long oldval)return ; }RCU_TRACE (trace_rcu_dyntick ("Start" , oldval, rcu_dynticks_nesting)); -if (current->pid !=0 ) { +if (!is_idle_task (current)) {struct task_struct *idle =idle_task (smp_processor_id ()); RCU_TRACE(trace_rcu_dyntick("Error on entry: not idle task",@@ -118,7 +118,7 @@ static void rcu_idle_exit_common(long long oldval)return ; }RCU_TRACE (trace_rcu_dyntick ("End" , oldval, rcu_dynticks_nesting)); -if (current->pid !=0 ) { +if (!is_idle_task (current)) {struct task_struct *idle =idle_task (smp_processor_id ()); RCU_TRACE(trace_rcu_dyntick("Error on exit: not idle task",kernel/rcutree.c +2
-2@@ -355,7 +355,7 @@ static void rcu_idle_enter_common(struct rcu_dynticks *rdtp, long long oldval)return ; }trace_rcu_dyntick ("Start" , oldval, rdtp->dynticks_nesting); -if (current->pid !=0 ) { +if (!is_idle_task (current)) {struct task_struct *idle =idle_task (smp_processor_id ()); trace_rcu_dyntick("Error on entry: not idle task",@@ -449,7 +449,7 @@ static void rcu_idle_exit_common(struct rcu_dynticks *rdtp, long long oldval)smp_mb__after_atomic_inc ();/* See above. */ WARN_ON_ONCE (!(atomic_read (&rdtp->dynticks) &0x1 ));trace_rcu_dyntick ("End" , oldval, rdtp->dynticks_nesting); -if (current->pid !=0 ) { +if (!is_idle_task (current)) {struct task_struct *idle =idle_task (smp_processor_id ()); trace_rcu_dyntick("Error on exit: not idle task",Signature · 1 line
1.7.3.2
PATCH 6/9 sparc: Make SPARC use the new is_idle_task() API
In reply to Paul E. McKenneyFrom: Paul E. McKenney <[email protected]>
Change from direct comparison of ->pid with zero to is_idle_task().
--- arch/sparc/kernel/setup_32.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
arch/sparc/kernel/setup_32.c +1 -1
@@ -84,7 +84,7 @@ static void prom_sync_me(void)prom_printf ("PROM SYNC COMMAND...\n" );show_free_areas (0 ); -if (current->pid !=0 ) { +if (!is_idle_task (current)) {local_irq_enable ();sys_sync ();local_irq_disable ();
Signature · 1 line
1.7.3.2
Re: [PATCH tip/core/rcu 6/9] sparc: Make SPARC use the new is_idle_task() API
From: "Paul E. McKenney" <[email protected]>
Date: Tue, 15 Nov 2011 12:28:02 -0800
Quoted 7 lines
From: Paul E. McKenney <[email protected]>
Change from direct comparison of ->pid with zero to
is_idle_task().Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>Cc "David S. Miller" <[email protected]>
PATCH 7/9 kdb: Make KDB use the new is_idle_task() API
In reply to Paul E. McKenneyFrom: Paul E. McKenney <[email protected]>
Change from direct comparison of ->pid with zero to is_idle_task().
--- kernel/debug/kdb/kdb_support.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/debug/kdb/kdb_support.c +1 -1
@@ -636,7 +636,7 @@ char kdb_task_state_char (const struct task_struct *p) (p->exit_state &EXIT_ZOMBIE ) ?'Z' : (p->exit_state &EXIT_DEAD ) ?'E' : (p->state &TASK_INTERRUPTIBLE ) ?'S' :'?' ; -if (p->pid ==0 ) { +if (is_idle_task (p)) {/* Idle task. Is it really idle, apart from the kdb * interrupt? */ if (!kdb_task_has_cpu (p) || kgdb_info[cpu].irq_depth ==1 ) {
Signature · 1 line
1.7.3.2
PATCH 8/9 events: Make events use the new is_idle_task() API
In reply to Paul E. McKenneyFrom: Paul E. McKenney <[email protected]>
Change from direct comparison of ->pid with zero to is_idle_task().
--- kernel/events/core.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/events/core.c +1 -1
@@ -5283,7 +5283,7 @@ static enum hrtimer_restart perf_swevent_hrtimer(struct hrtimer *hrtimer) regs =get_irq_regs ();if (regs && !perf_exclude_event (event, regs)) { -if (!(event->attr.exclude_idle && current->pid ==0 )) +if (!(event->attr.exclude_idle &&is_idle_task (current)))if (perf_event_overflow (event, &data, regs)) ret =HRTIMER_NORESTART ; }
Signature · 1 line
1.7.3.2
PATCH 9/9 tile: Make tile use the new is_idle_task() API
In reply to Paul E. McKenneyFrom: Paul E. McKenney <[email protected]>
Change from direct comparison of ->pid with zero to is_idle_task().
--- arch/tile/mm/fault.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
arch/tile/mm/fault.c +2 -2
@@ -54,7 +54,7 @@ static noinline void force_sig_info_fault(const char *type, int si_signo,if (unlikely (tsk->pid <2 )) {panic ("Signal %d (code %d) at %#lx sent to %s!" , si_signo, si_code &0xffff , address, - tsk->pid ?"init" :"the idle task" ); +is_idle_task (tsk) ?"the idle task" :"init" ); } info.si_signo = si_signo;
@@ -515,7 +515,7 @@ no_context:if (unlikely (tsk->pid <2 )) {panic ("Kernel page fault running %s!" , - tsk->pid ?"init" :"the idle task" ); +is_idle_task (tsk) ?"the idle task" :"init" ); }/*
Signature · 1 line
1.7.3.2
Re: [PATCH tip/core/rcu 9/9] tile: Make tile use the new is_idle_task() API
(Narrowing cc's)
Quoted 9 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
Change from direct comparison of ->pid with zero to
is_idle_task().Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>--- arch/tile/mm/fault.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
Please cc me on changes to tile architecture files - thanks! I picked this up while belatedly catching up on LKML.
Signature · 2 lines
Chris Metcalf, Tilera Corp. http://www.tilera.com
Re: [PATCH tip/core/rcu 9/9] tile: Make tile use the new is_idle_task() API
Quoted 13 lines · Chris Metcalf
(Narrowing cc's)
Quoted 9 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
Change from direct comparison of ->pid with zero to
is_idle_task().Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>--- arch/tile/mm/fault.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
Quoted 4 lines
Acked-by Chris Metcalf <[email protected]>Please cc me on changes to tile architecture files - thanks! I picked this up while belatedly catching up on LKML.
My apologies -- I have added your Acked-by (thank you!) and will try harder to get the CCs right in the future.
Thanx, Paul
Quoted 9 lines · Paul E. McKenney
This patchset is in addition to the set previously posted (see
https://lkml.org/lkml/2011/11/2/363). This new set permits the reworkedRCU_FAST_NO_HZcode to be used fromTREE_PREEMPT_RCU, allows rcutorture to shut down the system after a fixed time period (useful for KVM-based testing), adds an API to replace the "->pid == 0" discussed in response to the earlier patch set, and applies this API where applicable. The patches are as follows:
Quoted 9 lines
1. Update the
RCU_FAST_NO_HZconfig definition to allow it to
be used inTREE_PREEMPT_RCUbuilds, since that is now legal.
2. Add a module parameter that permit rcutorture to shut down the
system cleanly after the test has run for the specified time.
3. Add a module parameter to allow rcutorture start running
as soon as it is initialized.
4. Addis_idle_task()API to replace open-coded tests of ->pid
against zero.
5-9. Apply the newis_idle_task()API where it makes sense.
I replied to patches 2-4 with review comments. For the rest:
Quoted 20 lines · Josh Triplett
Quoted 9 lines · Paul E. McKenney
This patchset is in addition to the set previously posted (see
https://lkml.org/lkml/2011/11/2/363). This new set permits the reworkedRCU_FAST_NO_HZcode to be used fromTREE_PREEMPT_RCU, allows rcutorture to shut down the system after a fixed time period (useful for KVM-based testing), adds an API to replace the "->pid == 0" discussed in response to the earlier patch set, and applies this API where applicable. The patches are as follows:Quoted 9 lines
1. Update the
RCU_FAST_NO_HZconfig definition to allow it to
be used inTREE_PREEMPT_RCUbuilds, since that is now legal.
2. Add a module parameter that permit rcutorture to shut down the
system cleanly after the test has run for the specified time.
3. Add a module parameter to allow rcutorture start running
as soon as it is initialized.
4. Addis_idle_task()API to replace open-coded tests of ->pid
against zero.
5-9. Apply the newis_idle_task()API where it makes sense.
Quoted 3 lines
I replied to patches 2-4 with review comments. For the rest:
Reviewed-by Josh Triplett <[email protected]>
Thank you for your careful review and thoughtful comments!
Thanx, Paul