[PATCH tip/core/rcu 0/9] Preview of additional RCU changes for 3.3

[PATCH tip/core/rcu 0/9] Preview of additional RCU changes for 3.3

[Paul E. McKenney]

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 tip/core/rcu 1/9] rcu: Permit RCU_FAST_NO_HZ to be used by TREE_PREEMPT_RCU

[Paul E. McKenney]

From: Paul E. McKenney <paul.mckenney@linaro.org>

The new implementation of RCU_FAST_NO_HZ is compatible with preemptible
RCU, so this commit removes the Kconfig restriction that previously
prohibited this.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 init/Kconfig |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 43298f9..82b6a4c 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -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.
-- 
1.7.3.2

[PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability

[Paul E. McKenney]

From: Paul E. McKenney <paul.mckenney@linaro.org>

Although it is easy to run rcutorture tests under KVM, there is currently
no nice way to run such a test for a fixed time period, collect all of
the rcutorture data, and then shut the system down cleanly.  This commit
therefore adds an rcutorture module parameter named "shutdown_secs" that
specified the run duration in seconds, after which rcutorture terminates
the test and powers the system down.  The default value for "shutdown_secs"
is zero, which disables shutdown.

Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 Documentation/RCU/torture.txt |    5 +++
 kernel/rcutorture.c           |   62 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/Documentation/RCU/torture.txt b/Documentation/RCU/torture.txt
index 783d6c1..af40929 100644
--- a/Documentation/RCU/torture.txt
+++ b/Documentation/RCU/torture.txt
@@ -66,6 +66,11 @@ shuffle_interval
 		to a particular subset of the CPUs, defaults to 3 seconds.
 		Used in conjunction with test_no_idle_hz.
 
+shutdown_secs	The number of seconds to run the test before terminating
+		the test and powering off the system.  The default is
+		zero, which disables test termination and system shutdown.
+		This capability is useful for automated testing.
+
 stat_interval	The number of seconds between output of torture
 		statistics (via printk()).  Regardless of the interval,
 		statistics are printed when the module is unloaded.
diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
index df35228..41802be 100644
--- a/kernel/rcutorture.c
+++ b/kernel/rcutorture.c
@@ -61,9 +61,10 @@ static int test_no_idle_hz;	/* Test RCU's support for tickless idle CPUs. */
 static int shuffle_interval = 3; /* Interval between shuffles (in sec)*/
 static int stutter = 5;		/* Start/stop testing interval (in sec) */
 static int irqreader = 1;	/* RCU readers from irq (timers). */
-static int fqs_duration = 0;	/* Duration of bursts (us), 0 to disable. */
-static int fqs_holdoff = 0;	/* Hold time within burst (us). */
+static int fqs_duration;	/* Duration of bursts (us), 0 to disable. */
+static int fqs_holdoff;		/* Hold time within burst (us). */
 static int fqs_stutter = 3;	/* Wait time between bursts (s). */
+static int shutdown_secs;	/* Shutdown time (s).  <=0 for no shutdown. */
 static int test_boost = 1;	/* Test RCU prio boost: 0=no, 1=maybe, 2=yes. */
 static int test_boost_interval = 7; /* Interval between boost tests, seconds. */
 static int test_boost_duration = 4; /* Duration of each boost test, seconds. */
@@ -91,6 +92,8 @@ module_param(fqs_holdoff, int, 0444);
 MODULE_PARM_DESC(fqs_holdoff, "Holdoff time within fqs bursts (us)");
 module_param(fqs_stutter, int, 0444);
 MODULE_PARM_DESC(fqs_stutter, "Wait time between fqs bursts (s)");
+module_param(shutdown_secs, int, 0444);
+MODULE_PARM_DESC(shutdown_secs, "Shutdown time (s), zero to disable.");
 module_param(test_boost, int, 0444);
 MODULE_PARM_DESC(test_boost, "Test RCU prio boost: 0=no, 1=maybe, 2=yes.");
 module_param(test_boost_interval, int, 0444);
@@ -119,6 +122,7 @@ static struct task_struct *shuffler_task;
 static struct task_struct *stutter_task;
 static struct task_struct *fqs_task;
 static struct task_struct *boost_tasks[NR_CPUS];
+static struct task_struct *shutdown_task;
 
 #define RCU_TORTURE_PIPE_LEN 10
 
@@ -167,6 +171,7 @@ int rcutorture_runnable = RCUTORTURE_RUNNABLE_INIT;
 #define rcu_can_boost() 0
 #endif /* #else #if defined(CONFIG_RCU_BOOST) && !defined(CONFIG_HOTPLUG_CPU) */
 
+static unsigned long shutdown_time;	/* jiffies to system shutdown. */
 static unsigned long boost_starttime;	/* jiffies of next boost test start. */
 DEFINE_MUTEX(boost_mutex);		/* protect setting boost_starttime */
 					/*  and boost task create/destroy. */
@@ -182,6 +187,9 @@ static int fullstop = FULLSTOP_RMMOD;
  */
 static DEFINE_MUTEX(fullstop_mutex);
 
+/* Forward reference. */
+static void rcu_torture_cleanup(void);
+
 /*
  * Detect and respond to a system shutdown.
  */
@@ -1250,12 +1258,12 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, char *tag)
 		"shuffle_interval=%d stutter=%d irqreader=%d "
 		"fqs_duration=%d fqs_holdoff=%d fqs_stutter=%d "
 		"test_boost=%d/%d test_boost_interval=%d "
-		"test_boost_duration=%d\n",
+		"test_boost_duration=%d shutdown_secs=%d\n",
 		torture_type, tag, nrealreaders, nfakewriters,
 		stat_interval, verbose, test_no_idle_hz, shuffle_interval,
 		stutter, irqreader, fqs_duration, fqs_holdoff, fqs_stutter,
 		test_boost, cur_ops->can_boost,
-		test_boost_interval, test_boost_duration);
+		test_boost_interval, test_boost_duration, shutdown_secs);
 }
 
 static struct notifier_block rcutorture_shutdown_nb = {
@@ -1305,6 +1313,37 @@ static int rcutorture_booster_init(int cpu)
 	return 0;
 }
 
+/*
+ * Cause the rcutorture test to "stutter", starting and stopping all
+ * threads periodically.
+ */
+static int
+rcu_torture_shutdown(void *arg)
+{
+	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
+	while (ULONG_CMP_LT(jiffies, shutdown_time) &&
+	       !kthread_should_stop()) {
+		if (verbose)
+			printk(KERN_ALERT "%s" TORTURE_FLAG
+			       "rcu_torture_shutdown task: %lu "
+			       "jiffies remaining\n",
+			       torture_type, shutdown_time - jiffies);
+		schedule_timeout_interruptible(HZ);
+	}
+	if (ULONG_CMP_LT(jiffies, shutdown_time)) {
+		VERBOSE_PRINTK_STRING("rcu_torture_shutdown task stopping");
+		return 0;
+	}
+
+	/* OK, shut down the system. */
+
+	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task shutting down system");
+	shutdown_task = NULL;	/* Avoid self-kill deadlock. */
+	rcu_torture_cleanup();	/* Get the success/failure message. */
+	kernel_power_off();	/* Shut down the system. */
+	return 0;
+}
+
 static int rcutorture_cpu_notify(struct notifier_block *self,
 				 unsigned long action, void *hcpu)
 {
@@ -1409,6 +1448,10 @@ rcu_torture_cleanup(void)
 		for_each_possible_cpu(i)
 			rcutorture_booster_cleanup(i);
 	}
+	if (shutdown_task != NULL) {
+		VERBOSE_PRINTK_STRING("Stopping rcu_torture_shutdown task");
+		kthread_stop(shutdown_task);
+	}
 
 	/* Wait for all RCU callbacks to fire.  */
 
@@ -1625,6 +1668,17 @@ rcu_torture_init(void)
 			}
 		}
 	}
+	if (shutdown_secs > 0) {
+		shutdown_time = jiffies + shutdown_secs * HZ;
+		shutdown_task = kthread_run(rcu_torture_shutdown, NULL,
+					    "rcu_torture_shutdown");
+		if (IS_ERR(shutdown_task)) {
+			firsterr = PTR_ERR(shutdown_task);
+			VERBOSE_PRINTK_ERRSTRING("Failed to create shutdown");
+			shutdown_task = NULL;
+			goto unwind;
+		}
+	}
 	register_reboot_notifier(&rcutorture_shutdown_nb);
 	rcutorture_record_test_transition();
 	mutex_unlock(&fullstop_mutex);
-- 
1.7.3.2

[PATCH tip/core/rcu 3/9] rcu: Control rcutorture startup from kernel boot parameters

[Paul E. McKenney]

From: Paul E. McKenney <paul.mckenney@linaro.org>

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 <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutorture.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
index 41802be..fd7a0e6 100644
--- a/kernel/rcutorture.c
+++ b/kernel/rcutorture.c
@@ -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
-- 
1.7.3.2

[PATCH tip/core/rcu 4/9] sched: add is_idle_task() to handle invalidated uses of idle_cpu()

[Paul E. McKenney]

From: Paul E. McKenney <paul.mckenney@linaro.org>

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.

Suggested-by: Josh Triplett <josh@joshtriplett.org>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 include/linux/sched.h |    1 +
 kernel/sched.c        |    9 +++++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 68daf4f..4b1077b 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -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);
 
diff --git a/kernel/sched.c b/kernel/sched.c
index 0e9344a..c9c1bb3 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -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.
  */
-- 
1.7.3.2

[PATCH tip/core/rcu 5/9] rcu: Make RCU use the new is_idle_task() API

[Paul E. McKenney]

From: Paul E. McKenney <paul.mckenney@linaro.org>

Change from direct comparison of ->pid with zero to is_idle_task().

Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutiny.c |    4 ++--
 kernel/rcutree.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
index 4e16ce3..e5bd949 100644
--- a/kernel/rcutiny.c
+++ b/kernel/rcutiny.c
@@ -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",
diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index 49e0783..7fb8b0e 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -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",
-- 
1.7.3.2

[PATCH tip/core/rcu 6/9] sparc: Make SPARC use the new is_idle_task() API

[Paul E. McKenney]

From: Paul E. McKenney <paul.mckenney@linaro.org>

Change from direct comparison of ->pid with zero to is_idle_task().

Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
 arch/sparc/kernel/setup_32.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/sparc/kernel/setup_32.c b/arch/sparc/kernel/setup_32.c
index fe1e3fc..ffb883d 100644
--- a/arch/sparc/kernel/setup_32.c
+++ b/arch/sparc/kernel/setup_32.c
@@ -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();
-- 
1.7.3.2

[PATCH tip/core/rcu 7/9] kdb: Make KDB use the new is_idle_task() API

[Paul E. McKenney]

From: Paul E. McKenney <paul.mckenney@linaro.org>

Change from direct comparison of ->pid with zero to is_idle_task().

Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Jason Wessel <jason.wessel@windriver.com>
---
 kernel/debug/kdb/kdb_support.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
index 5532dd3..7d6fb40 100644
--- a/kernel/debug/kdb/kdb_support.c
+++ b/kernel/debug/kdb/kdb_support.c
@@ -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) {
-- 
1.7.3.2

[PATCH tip/core/rcu 8/9] events: Make events use the new is_idle_task() API

[Paul E. McKenney]

From: Paul E. McKenney <paul.mckenney@linaro.org>

Change from direct comparison of ->pid with zero to is_idle_task().

Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
---
 kernel/events/core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 0e8457d..cd3aa34 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -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;
 	}
-- 
1.7.3.2

[PATCH tip/core/rcu 9/9] tile: Make tile use the new is_idle_task() API

[Paul E. McKenney]

From: Paul E. McKenney <paul.mckenney@linaro.org>

Change from direct comparison of ->pid with zero to is_idle_task().

Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 arch/tile/mm/fault.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/tile/mm/fault.c b/arch/tile/mm/fault.c
index 25b7b90..c1eaaa1 100644
--- a/arch/tile/mm/fault.c
+++ b/arch/tile/mm/fault.c
@@ -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");
 	}
 
 	/*
-- 
1.7.3.2

Re: [PATCH tip/core/rcu 4/9] sched: add is_idle_task() to handle invalidated uses of idle_cpu()

[Josh Triplett]

On Tue, Nov 15, 2011 at 12:28:00PM -0800, Paul E. McKenney wrote:
> From: Paul E. McKenney <paul.mckenney@linaro.org>
> 
> 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.
> 
> Suggested-by: Josh Triplett <josh@joshtriplett.org>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  include/linux/sched.h |    1 +
>  kernel/sched.c        |    9 +++++++++
>  2 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 68daf4f..4b1077b 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -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 6/9] sparc: Make SPARC use the new is_idle_task() API

[David Miller]

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Tue, 15 Nov 2011 12:28:02 -0800

> From: Paul E. McKenney <paul.mckenney@linaro.org>
> 
> Change from direct comparison of ->pid with zero to is_idle_task().
> 
> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: "David S. Miller" <davem@davemloft.net>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH tip/core/rcu 5/9] rcu: Make RCU use the new is_idle_task() API

[Josh Triplett]

On Tue, Nov 15, 2011 at 12:28:01PM -0800, Paul E. McKenney wrote:
> From: Paul E. McKenney <paul.mckenney@linaro.org>
> 
> Change from direct comparison of ->pid with zero to is_idle_task().
> 
> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  kernel/rcutiny.c |    4 ++--
>  kernel/rcutree.c |    4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
> index 4e16ce3..e5bd949 100644
> --- a/kernel/rcutiny.c
> +++ b/kernel/rcutiny.c
> @@ -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",
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index 49e0783..7fb8b0e 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -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",
> -- 
> 1.7.3.2
> 

Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability

[Josh Triplett]

On Tue, Nov 15, 2011 at 12:27:58PM -0800, Paul E. McKenney wrote:
> From: Paul E. McKenney <paul.mckenney@linaro.org>
> 
> Although it is easy to run rcutorture tests under KVM, there is currently
> no nice way to run such a test for a fixed time period, collect all of
> the rcutorture data, and then shut the system down cleanly.  This commit
> therefore adds an rcutorture module parameter named "shutdown_secs" that
> specified the run duration in seconds, after which rcutorture terminates
> the test and powers the system down.  The default value for "shutdown_secs"
> is zero, which disables shutdown.
> 
> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

>From your recent post on this, I thought you found a solution through
the init= parameter, which seems preferable.

> --- a/kernel/rcutorture.c
> +++ b/kernel/rcutorture.c
> @@ -61,9 +61,10 @@ static int test_no_idle_hz;	/* Test RCU's support for tickless idle CPUs. */
>  static int shuffle_interval = 3; /* Interval between shuffles (in sec)*/
>  static int stutter = 5;		/* Start/stop testing interval (in sec) */
>  static int irqreader = 1;	/* RCU readers from irq (timers). */
> -static int fqs_duration = 0;	/* Duration of bursts (us), 0 to disable. */
> -static int fqs_holdoff = 0;	/* Hold time within burst (us). */
> +static int fqs_duration;	/* Duration of bursts (us), 0 to disable. */
> +static int fqs_holdoff;		/* Hold time within burst (us). */

Looks like these lines picked up unrelated whitespace changes in this
commit.

> @@ -1305,6 +1313,37 @@ static int rcutorture_booster_init(int cpu)
>  	return 0;
>  }
>  
> +/*
> + * Cause the rcutorture test to "stutter", starting and stopping all
> + * threads periodically.
> + */

This comment looks like a copy-paste error.

> +static int
> +rcu_torture_shutdown(void *arg)
> +{
> +	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> +	while (ULONG_CMP_LT(jiffies, shutdown_time) &&
> +	       !kthread_should_stop()) {
> +		if (verbose)
> +			printk(KERN_ALERT "%s" TORTURE_FLAG
> +			       "rcu_torture_shutdown task: %lu "
> +			       "jiffies remaining\n",
> +			       torture_type, shutdown_time - jiffies);
> +		schedule_timeout_interruptible(HZ);
> +	}

Any particular reason to wake up once a second here?  If !verbose, this could just
sleep until shutdown time.  (And does the verbose output really help
here, given printk timestamps?)

> +	if (ULONG_CMP_LT(jiffies, shutdown_time)) {
> +		VERBOSE_PRINTK_STRING("rcu_torture_shutdown task stopping");
> +		return 0;
> +	}
> +
> +	/* OK, shut down the system. */
> +
> +	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task shutting down system");
> +	shutdown_task = NULL;	/* Avoid self-kill deadlock. */

Not that it matters much here, but won't this cause a leak?

> +	rcu_torture_cleanup();	/* Get the success/failure message. */
> +	kernel_power_off();	/* Shut down the system. */
> +	return 0;
> +}

Huh.  I would have expected kernel_power_off to use noreturn, making the
return 0 unnecessary here; however, apparently it doesn't.

- Josh Triplett

Re: [PATCH tip/core/rcu 3/9] rcu: Control rcutorture startup from kernel boot parameters

[Josh Triplett]

On Tue, Nov 15, 2011 at 12:27:59PM -0800, Paul E. McKenney wrote:
> From: Paul E. McKenney <paul.mckenney@linaro.org>
> 
> 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 <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  kernel/rcutorture.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
> index 41802be..fd7a0e6 100644
> --- a/kernel/rcutorture.c
> +++ b/kernel/rcutorture.c
> @@ -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 0/9] Preview of additional RCU changes for 3.3

[Josh Triplett]

On Tue, Nov 15, 2011 at 12:27:36PM -0800, Paul E. McKenney wrote:
> 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.

I replied to patches 2-4 with review comments.  For the rest:

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

Re: [PATCH tip/core/rcu 4/9] sched: add is_idle_task() to handle invalidated uses of idle_cpu()

[Paul E. McKenney]

On Tue, Nov 15, 2011 at 01:13:02PM -0800, Josh Triplett wrote:
> On Tue, Nov 15, 2011 at 12:28:00PM -0800, Paul E. McKenney wrote:
> > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > 
> > 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.
> > 
> > Suggested-by: Josh Triplett <josh@joshtriplett.org>
> > Suggested-by: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >  include/linux/sched.h |    1 +
> >  kernel/sched.c        |    9 +++++++++
> >  2 files changed, 10 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 68daf4f..4b1077b 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -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?

That does work for all the uses thus far, so made both suggested changes.

							Thanx, Paul

Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability

[Paul E. McKenney]

On Tue, Nov 15, 2011 at 01:46:15PM -0800, Josh Triplett wrote:
> On Tue, Nov 15, 2011 at 12:27:58PM -0800, Paul E. McKenney wrote:
> > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > 
> > Although it is easy to run rcutorture tests under KVM, there is currently
> > no nice way to run such a test for a fixed time period, collect all of
> > the rcutorture data, and then shut the system down cleanly.  This commit
> > therefore adds an rcutorture module parameter named "shutdown_secs" that
> > specified the run duration in seconds, after which rcutorture terminates
> > the test and powers the system down.  The default value for "shutdown_secs"
> > is zero, which disables shutdown.
> > 
> > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> >From your recent post on this, I thought you found a solution through
> the init= parameter, which seems preferable.

For some things, the init= parameter does work great.  I do intend to
use it when collecting event-tracing and debugfs data, for example.

However, there is still a need for RCU torture testing that will operate
correctly regardless of how userspace is set up.  That, and there are
quite a few different kernel test setup, each with their own peculiar
capabilities and limitations.  So what happened was that before people
suggested the init= approach, I implemented enough of the in-kernel
approach to appreciate how much it simplifies life for the common case of
"just torture-test RCU".  As in I should have done this long ago.

I will therefore be taking both approaches.  There will be at least one
more patch pushing what is now script into rcutorture.c.

> > --- a/kernel/rcutorture.c
> > +++ b/kernel/rcutorture.c
> > @@ -61,9 +61,10 @@ static int test_no_idle_hz;	/* Test RCU's support for tickless idle CPUs. */
> >  static int shuffle_interval = 3; /* Interval between shuffles (in sec)*/
> >  static int stutter = 5;		/* Start/stop testing interval (in sec) */
> >  static int irqreader = 1;	/* RCU readers from irq (timers). */
> > -static int fqs_duration = 0;	/* Duration of bursts (us), 0 to disable. */
> > -static int fqs_holdoff = 0;	/* Hold time within burst (us). */
> > +static int fqs_duration;	/* Duration of bursts (us), 0 to disable. */
> > +static int fqs_holdoff;		/* Hold time within burst (us). */
> 
> Looks like these lines picked up unrelated whitespace changes in this
> commit.

Turns out that my initial patch added another variable that I explicitly
initialized to zero.  Of course, checkpatch yelled at me about this, so
I figured I should fix the other nearby occurrences of this while I was
at it.  Doesn't really seem to me to be worth a separate patch, though.

> > @@ -1305,6 +1313,37 @@ static int rcutorture_booster_init(int cpu)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Cause the rcutorture test to "stutter", starting and stopping all
> > + * threads periodically.
> > + */
> 
> This comment looks like a copy-paste error.

Or maybe a copy-paste stutter.  ;-)

Good eyes, fixed!

> > +static int
> > +rcu_torture_shutdown(void *arg)
> > +{
> > +	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> > +	while (ULONG_CMP_LT(jiffies, shutdown_time) &&
> > +	       !kthread_should_stop()) {
> > +		if (verbose)
> > +			printk(KERN_ALERT "%s" TORTURE_FLAG
> > +			       "rcu_torture_shutdown task: %lu "
> > +			       "jiffies remaining\n",
> > +			       torture_type, shutdown_time - jiffies);
> > +		schedule_timeout_interruptible(HZ);
> > +	}
> 
> Any particular reason to wake up once a second here?  If !verbose, this could just
> sleep until shutdown time.  (And does the verbose output really help
> here, given printk timestamps?)

It actually did help me find a bug where it was failing to shut down.
I could use different code paths, but that would defeat the debugging.

So I increased the sleep time to 30 seconds.  Fair enough?

> > +	if (ULONG_CMP_LT(jiffies, shutdown_time)) {
> > +		VERBOSE_PRINTK_STRING("rcu_torture_shutdown task stopping");
> > +		return 0;
> > +	}
> > +
> > +	/* OK, shut down the system. */
> > +
> > +	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task shutting down system");
> > +	shutdown_task = NULL;	/* Avoid self-kill deadlock. */
> 
> Not that it matters much here, but won't this cause a leak?

Only if we are shutting down.  And the alternative is a deadlock
where this task invokes kthread_stop() on itself.  ;-)

> > +	rcu_torture_cleanup();	/* Get the success/failure message. */
> > +	kernel_power_off();	/* Shut down the system. */
> > +	return 0;
> > +}
> 
> Huh.  I would have expected kernel_power_off to use noreturn, making the
> return 0 unnecessary here; however, apparently it doesn't.

Indeed, gcc yelled at me, so I added the "return 0".  ;-)

							Thanx, Paul

Re: [PATCH tip/core/rcu 3/9] rcu: Control rcutorture startup from kernel boot parameters

[Paul E. McKenney]

On Tue, Nov 15, 2011 at 01:49:32PM -0800, Josh Triplett wrote:
> On Tue, Nov 15, 2011 at 12:27:59PM -0800, Paul E. McKenney wrote:
> > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > 
> > 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 <paul.mckenney@linaro.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >  kernel/rcutorture.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
> > index 41802be..fd7a0e6 100644
> > --- a/kernel/rcutorture.c
> > +++ b/kernel/rcutorture.c
> > @@ -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

Re: [PATCH tip/core/rcu 0/9] Preview of additional RCU changes for 3.3

[Paul E. McKenney]

On Tue, Nov 15, 2011 at 01:50:41PM -0800, Josh Triplett wrote:
> On Tue, Nov 15, 2011 at 12:27:36PM -0800, Paul E. McKenney wrote:
> > 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.
> 
> I replied to patches 2-4 with review comments.  For the rest:
> 
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>

Thank you for your careful review and thoughtful comments!

							Thanx, Paul

Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability

[Josh Triplett]

On Wed, Nov 16, 2011 at 12:32:26PM -0800, Paul E. McKenney wrote:
> On Tue, Nov 15, 2011 at 01:46:15PM -0800, Josh Triplett wrote:
> > On Tue, Nov 15, 2011 at 12:27:58PM -0800, Paul E. McKenney wrote:
> > > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > > 
> > > Although it is easy to run rcutorture tests under KVM, there is currently
> > > no nice way to run such a test for a fixed time period, collect all of
> > > the rcutorture data, and then shut the system down cleanly.  This commit
> > > therefore adds an rcutorture module parameter named "shutdown_secs" that
> > > specified the run duration in seconds, after which rcutorture terminates
> > > the test and powers the system down.  The default value for "shutdown_secs"
> > > is zero, which disables shutdown.
> > > 
> > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > >From your recent post on this, I thought you found a solution through
> > the init= parameter, which seems preferable.
> 
> For some things, the init= parameter does work great.  I do intend to
> use it when collecting event-tracing and debugfs data, for example.
> 
> However, there is still a need for RCU torture testing that will operate
> correctly regardless of how userspace is set up.  That, and there are
> quite a few different kernel test setup, each with their own peculiar
> capabilities and limitations.  So what happened was that before people
> suggested the init= approach, I implemented enough of the in-kernel
> approach to appreciate how much it simplifies life for the common case of
> "just torture-test RCU".  As in I should have done this long ago.

Seems like it would work just as easily to point init at a statically
linked C program which just sleeps for a fixed time and then shuts down.
However, given the special-purpose nature of rcutorture, I won't
complain that strongly.

> > > --- a/kernel/rcutorture.c
> > > +++ b/kernel/rcutorture.c
> > > @@ -61,9 +61,10 @@ static int test_no_idle_hz;	/* Test RCU's support for tickless idle CPUs. */
> > >  static int shuffle_interval = 3; /* Interval between shuffles (in sec)*/
> > >  static int stutter = 5;		/* Start/stop testing interval (in sec) */
> > >  static int irqreader = 1;	/* RCU readers from irq (timers). */
> > > -static int fqs_duration = 0;	/* Duration of bursts (us), 0 to disable. */
> > > -static int fqs_holdoff = 0;	/* Hold time within burst (us). */
> > > +static int fqs_duration;	/* Duration of bursts (us), 0 to disable. */
> > > +static int fqs_holdoff;		/* Hold time within burst (us). */
> > 
> > Looks like these lines picked up unrelated whitespace changes in this
> > commit.
> 
> Turns out that my initial patch added another variable that I explicitly
> initialized to zero.  Of course, checkpatch yelled at me about this, so
> I figured I should fix the other nearby occurrences of this while I was
> at it.  Doesn't really seem to me to be worth a separate patch, though.

Ah, I missed the removal of the initializer.  However, I don't see the
harm in splitting out the trivial two-line patch, rather than folding it
into an unrelated change which just happens to change lines nearby.

> > > +static int
> > > +rcu_torture_shutdown(void *arg)
> > > +{
> > > +	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> > > +	while (ULONG_CMP_LT(jiffies, shutdown_time) &&
> > > +	       !kthread_should_stop()) {
> > > +		if (verbose)
> > > +			printk(KERN_ALERT "%s" TORTURE_FLAG
> > > +			       "rcu_torture_shutdown task: %lu "
> > > +			       "jiffies remaining\n",
> > > +			       torture_type, shutdown_time - jiffies);
> > > +		schedule_timeout_interruptible(HZ);
> > > +	}
> > 
> > Any particular reason to wake up once a second here?  If !verbose, this could just
> > sleep until shutdown time.  (And does the verbose output really help
> > here, given printk timestamps?)
> 
> It actually did help me find a bug where it was failing to shut down.
> I could use different code paths, but that would defeat the debugging.
> 
> So I increased the sleep time to 30 seconds.  Fair enough?

Well, now that you've debugged rcutorture's shutdown routine, would it
suffice to have a printk when you actually go to shut down, without
waking up for previous printks when not shutting down yet?

(The poll time doesn't really matter, and sleeping for 30 seconds before
checking the time means you might overshoot by up to 30 seconds.  I'd
like to avoid polling to begin with when you know exactly how long you
need to sleep.)

> > > +	if (ULONG_CMP_LT(jiffies, shutdown_time)) {
> > > +		VERBOSE_PRINTK_STRING("rcu_torture_shutdown task stopping");
> > > +		return 0;
> > > +	}
> > > +
> > > +	/* OK, shut down the system. */
> > > +
> > > +	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task shutting down system");
> > > +	shutdown_task = NULL;	/* Avoid self-kill deadlock. */
> > 
> > Not that it matters much here, but won't this cause a leak?
> 
> Only if we are shutting down.  And the alternative is a deadlock
> where this task invokes kthread_stop() on itself.  ;-)

Hence why I said it didn't matter much. :)

- Josh Triplett

Re: [PATCH tip/core/rcu 3/9] rcu: Control rcutorture startup from kernel boot parameters

[Josh Triplett]

On Wed, Nov 16, 2011 at 12:38:51PM -0800, Paul E. McKenney wrote:
> On Tue, Nov 15, 2011 at 01:49:32PM -0800, Josh Triplett wrote:
> > On Tue, Nov 15, 2011 at 12:27:59PM -0800, Paul E. McKenney wrote:
> > > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > > 
> > > 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 <paul.mckenney@linaro.org>
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > ---
> > >  kernel/rcutorture.c |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
> > > index 41802be..fd7a0e6 100644
> > > --- a/kernel/rcutorture.c
> > > +++ b/kernel/rcutorture.c
> > > @@ -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.

Ah, fair enough.

- Josh Triplett

Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability

[Paul E. McKenney]

On Wed, Nov 16, 2011 at 02:15:45PM -0800, Josh Triplett wrote:
> On Wed, Nov 16, 2011 at 12:32:26PM -0800, Paul E. McKenney wrote:
> > On Tue, Nov 15, 2011 at 01:46:15PM -0800, Josh Triplett wrote:
> > > On Tue, Nov 15, 2011 at 12:27:58PM -0800, Paul E. McKenney wrote:
> > > > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > 
> > > > Although it is easy to run rcutorture tests under KVM, there is currently
> > > > no nice way to run such a test for a fixed time period, collect all of
> > > > the rcutorture data, and then shut the system down cleanly.  This commit
> > > > therefore adds an rcutorture module parameter named "shutdown_secs" that
> > > > specified the run duration in seconds, after which rcutorture terminates
> > > > the test and powers the system down.  The default value for "shutdown_secs"
> > > > is zero, which disables shutdown.
> > > > 
> > > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > 
> > > >From your recent post on this, I thought you found a solution through
> > > the init= parameter, which seems preferable.
> > 
> > For some things, the init= parameter does work great.  I do intend to
> > use it when collecting event-tracing and debugfs data, for example.
> > 
> > However, there is still a need for RCU torture testing that will operate
> > correctly regardless of how userspace is set up.  That, and there are
> > quite a few different kernel test setup, each with their own peculiar
> > capabilities and limitations.  So what happened was that before people
> > suggested the init= approach, I implemented enough of the in-kernel
> > approach to appreciate how much it simplifies life for the common case of
> > "just torture-test RCU".  As in I should have done this long ago.
> 
> Seems like it would work just as easily to point init at a statically
> linked C program which just sleeps for a fixed time and then shuts down.
> However, given the special-purpose nature of rcutorture, I won't
> complain that strongly.

I did consider a statically linked C program, but that can introduce the
need for cross-compilation into situations that do not otherwise need it.

> > > > --- a/kernel/rcutorture.c
> > > > +++ b/kernel/rcutorture.c
> > > > @@ -61,9 +61,10 @@ static int test_no_idle_hz;	/* Test RCU's support for tickless idle CPUs. */
> > > >  static int shuffle_interval = 3; /* Interval between shuffles (in sec)*/
> > > >  static int stutter = 5;		/* Start/stop testing interval (in sec) */
> > > >  static int irqreader = 1;	/* RCU readers from irq (timers). */
> > > > -static int fqs_duration = 0;	/* Duration of bursts (us), 0 to disable. */
> > > > -static int fqs_holdoff = 0;	/* Hold time within burst (us). */
> > > > +static int fqs_duration;	/* Duration of bursts (us), 0 to disable. */
> > > > +static int fqs_holdoff;		/* Hold time within burst (us). */
> > > 
> > > Looks like these lines picked up unrelated whitespace changes in this
> > > commit.
> > 
> > Turns out that my initial patch added another variable that I explicitly
> > initialized to zero.  Of course, checkpatch yelled at me about this, so
> > I figured I should fix the other nearby occurrences of this while I was
> > at it.  Doesn't really seem to me to be worth a separate patch, though.
> 
> Ah, I missed the removal of the initializer.  However, I don't see the
> harm in splitting out the trivial two-line patch, rather than folding it
> into an unrelated change which just happens to change lines nearby.

Ummm...  Laziness on my part?  ;-)

> > > > +static int
> > > > +rcu_torture_shutdown(void *arg)
> > > > +{
> > > > +	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> > > > +	while (ULONG_CMP_LT(jiffies, shutdown_time) &&
> > > > +	       !kthread_should_stop()) {
> > > > +		if (verbose)
> > > > +			printk(KERN_ALERT "%s" TORTURE_FLAG
> > > > +			       "rcu_torture_shutdown task: %lu "
> > > > +			       "jiffies remaining\n",
> > > > +			       torture_type, shutdown_time - jiffies);
> > > > +		schedule_timeout_interruptible(HZ);
> > > > +	}
> > > 
> > > Any particular reason to wake up once a second here?  If !verbose, this could just
> > > sleep until shutdown time.  (And does the verbose output really help
> > > here, given printk timestamps?)
> > 
> > It actually did help me find a bug where it was failing to shut down.
> > I could use different code paths, but that would defeat the debugging.
> > 
> > So I increased the sleep time to 30 seconds.  Fair enough?
> 
> Well, now that you've debugged rcutorture's shutdown routine, would it
> suffice to have a printk when you actually go to shut down, without
> waking up for previous printks when not shutting down yet?
> 
> (The poll time doesn't really matter, and sleeping for 30 seconds before
> checking the time means you might overshoot by up to 30 seconds.  I'd
> like to avoid polling to begin with when you know exactly how long you
> need to sleep.)

Indeed, good points!  But please see below for what this function turns
into when taking that approach.

> > > > +	if (ULONG_CMP_LT(jiffies, shutdown_time)) {
> > > > +		VERBOSE_PRINTK_STRING("rcu_torture_shutdown task stopping");
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	/* OK, shut down the system. */
> > > > +
> > > > +	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task shutting down system");
> > > > +	shutdown_task = NULL;	/* Avoid self-kill deadlock. */
> > > 
> > > Not that it matters much here, but won't this cause a leak?
> > 
> > Only if we are shutting down.  And the alternative is a deadlock
> > where this task invokes kthread_stop() on itself.  ;-)
> 
> Hence why I said it didn't matter much. :)

;-)

								Thanx, Paul

------------------------------------------------------------------------

rcu_torture_shutdown(void *arg)
{
	long delta;
	unsigned long jiffies_snap;

	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
	jiffies_snap = ACCESS_ONCE(jiffies);
	while (ULONG_CMP_LT(jiffies_snap, shutdown_time) &&
	       !kthread_should_stop()) {
		delta = shutdown_time - jiffies_snap;
		if (verbose)
			printk(KERN_ALERT "%s" TORTURE_FLAG
			       "rcu_torture_shutdown task: %lu "
			       "jiffies remaining\n",
			       torture_type, delta);
		schedule_timeout_interruptible(delta);
		jiffies_snap = ACCESS_ONCE(jiffies);
	}
	if (ULONG_CMP_LT(jiffies_snap, shutdown_time)) {
		VERBOSE_PRINTK_STRING("rcu_torture_shutdown task stopping");
		return 0;
	}

	/* OK, shut down the system. */

	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task shutting down system");
	shutdown_task = NULL;	/* Avoid self-kill deadlock. */
	rcu_torture_cleanup();	/* Get the success/failure message. */
	kernel_power_off();	/* Shut down the system. */
	return 0;
}

Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability

[Josh Triplett]

On Wed, Nov 16, 2011 at 02:44:47PM -0800, Paul E. McKenney wrote:
> On Wed, Nov 16, 2011 at 02:15:45PM -0800, Josh Triplett wrote:
> > On Wed, Nov 16, 2011 at 12:32:26PM -0800, Paul E. McKenney wrote:
> > > On Tue, Nov 15, 2011 at 01:46:15PM -0800, Josh Triplett wrote:
> > > > On Tue, Nov 15, 2011 at 12:27:58PM -0800, Paul E. McKenney wrote:
> > > > > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > 
> > > > > Although it is easy to run rcutorture tests under KVM, there is currently
> > > > > no nice way to run such a test for a fixed time period, collect all of
> > > > > the rcutorture data, and then shut the system down cleanly.  This commit
> > > > > therefore adds an rcutorture module parameter named "shutdown_secs" that
> > > > > specified the run duration in seconds, after which rcutorture terminates
> > > > > the test and powers the system down.  The default value for "shutdown_secs"
> > > > > is zero, which disables shutdown.
> > > > > 
> > > > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > 
> > > > >From your recent post on this, I thought you found a solution through
> > > > the init= parameter, which seems preferable.
> > > 
> > > For some things, the init= parameter does work great.  I do intend to
> > > use it when collecting event-tracing and debugfs data, for example.
> > > 
> > > However, there is still a need for RCU torture testing that will operate
> > > correctly regardless of how userspace is set up.  That, and there are
> > > quite a few different kernel test setup, each with their own peculiar
> > > capabilities and limitations.  So what happened was that before people
> > > suggested the init= approach, I implemented enough of the in-kernel
> > > approach to appreciate how much it simplifies life for the common case of
> > > "just torture-test RCU".  As in I should have done this long ago.
> > 
> > Seems like it would work just as easily to point init at a statically
> > linked C program which just sleeps for a fixed time and then shuts down.
> > However, given the special-purpose nature of rcutorture, I won't
> > complain that strongly.
> 
> I did consider a statically linked C program, but that can introduce the
> need for cross-compilation into situations that do not otherwise need it.

Wouldn't you need to cross-compile the kernel anyway in such situations?

> > > > > +static int
> > > > > +rcu_torture_shutdown(void *arg)
> > > > > +{
> > > > > +	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> > > > > +	while (ULONG_CMP_LT(jiffies, shutdown_time) &&
> > > > > +	       !kthread_should_stop()) {
> > > > > +		if (verbose)
> > > > > +			printk(KERN_ALERT "%s" TORTURE_FLAG
> > > > > +			       "rcu_torture_shutdown task: %lu "
> > > > > +			       "jiffies remaining\n",
> > > > > +			       torture_type, shutdown_time - jiffies);
> > > > > +		schedule_timeout_interruptible(HZ);
> > > > > +	}
> > > > 
> > > > Any particular reason to wake up once a second here?  If !verbose, this could just
> > > > sleep until shutdown time.  (And does the verbose output really help
> > > > here, given printk timestamps?)
> > > 
> > > It actually did help me find a bug where it was failing to shut down.
> > > I could use different code paths, but that would defeat the debugging.
> > > 
> > > So I increased the sleep time to 30 seconds.  Fair enough?
> > 
> > Well, now that you've debugged rcutorture's shutdown routine, would it
> > suffice to have a printk when you actually go to shut down, without
> > waking up for previous printks when not shutting down yet?
> > 
> > (The poll time doesn't really matter, and sleeping for 30 seconds before
> > checking the time means you might overshoot by up to 30 seconds.  I'd
> > like to avoid polling to begin with when you know exactly how long you
> > need to sleep.)
> 
> Indeed, good points!  But please see below for what this function turns
> into when taking that approach.

See below for responses; that version seems like an improvement, though
it could still improve further.

> rcu_torture_shutdown(void *arg)
> {
> 	long delta;
> 	unsigned long jiffies_snap;
> 
> 	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> 	jiffies_snap = ACCESS_ONCE(jiffies);

Why do you need to snapshot jiffies in this version but not in the
version you originally posted?

> 	while (ULONG_CMP_LT(jiffies_snap, shutdown_time) &&
> 	       !kthread_should_stop()) {
> 		delta = shutdown_time - jiffies_snap;
> 		if (verbose)
> 			printk(KERN_ALERT "%s" TORTURE_FLAG
> 			       "rcu_torture_shutdown task: %lu "
> 			       "jiffies remaining\n",
> 			       torture_type, delta);

I suggested dropping this print entirely; under normal circumstances it
should never print.  It will only print if
schedule_timeout_interruptible wakes up spuriously.

> 		schedule_timeout_interruptible(delta);
> 		jiffies_snap = ACCESS_ONCE(jiffies);
> 	}

Any reason this entire loop body couldn't just become
msleep_interruptible()?

> 	if (ULONG_CMP_LT(jiffies_snap, shutdown_time)) {
> 		VERBOSE_PRINTK_STRING("rcu_torture_shutdown task stopping");
> 		return 0;
> 	}

Writing this as "if (kthread_should_stop())" seems clearer.

- Josh Triplett

Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability

[Paul E. McKenney]

On Wed, Nov 16, 2011 at 02:58:56PM -0800, Josh Triplett wrote:
> On Wed, Nov 16, 2011 at 02:44:47PM -0800, Paul E. McKenney wrote:
> > On Wed, Nov 16, 2011 at 02:15:45PM -0800, Josh Triplett wrote:
> > > On Wed, Nov 16, 2011 at 12:32:26PM -0800, Paul E. McKenney wrote:
> > > > On Tue, Nov 15, 2011 at 01:46:15PM -0800, Josh Triplett wrote:
> > > > > On Tue, Nov 15, 2011 at 12:27:58PM -0800, Paul E. McKenney wrote:
> > > > > > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > > 
> > > > > > Although it is easy to run rcutorture tests under KVM, there is currently
> > > > > > no nice way to run such a test for a fixed time period, collect all of
> > > > > > the rcutorture data, and then shut the system down cleanly.  This commit
> > > > > > therefore adds an rcutorture module parameter named "shutdown_secs" that
> > > > > > specified the run duration in seconds, after which rcutorture terminates
> > > > > > the test and powers the system down.  The default value for "shutdown_secs"
> > > > > > is zero, which disables shutdown.
> > > > > > 
> > > > > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > 
> > > > > >From your recent post on this, I thought you found a solution through
> > > > > the init= parameter, which seems preferable.
> > > > 
> > > > For some things, the init= parameter does work great.  I do intend to
> > > > use it when collecting event-tracing and debugfs data, for example.
> > > > 
> > > > However, there is still a need for RCU torture testing that will operate
> > > > correctly regardless of how userspace is set up.  That, and there are
> > > > quite a few different kernel test setup, each with their own peculiar
> > > > capabilities and limitations.  So what happened was that before people
> > > > suggested the init= approach, I implemented enough of the in-kernel
> > > > approach to appreciate how much it simplifies life for the common case of
> > > > "just torture-test RCU".  As in I should have done this long ago.
> > > 
> > > Seems like it would work just as easily to point init at a statically
> > > linked C program which just sleeps for a fixed time and then shuts down.
> > > However, given the special-purpose nature of rcutorture, I won't
> > > complain that strongly.
> > 
> > I did consider a statically linked C program, but that can introduce the
> > need for cross-compilation into situations that do not otherwise need it.
> 
> Wouldn't you need to cross-compile the kernel anyway in such situations?

Not necessarily, consider for example ABAT.  (IBM-specific test setup
for those unfamiliar with it -- related to autotest.)

I suspect that the only way for you to be convinced is for you to write
a script that takes your preferred approach for injecting a test into
(say) a KVM instance.

Then compare that script to adding a few parameters to the boot line,
namely: "rcutorture.stat_interval=15 rcutorture.shutdown_secs=3600
rcutorture.rcutorture_runnable=1".  ;-)

Using the parameters allows me to not care about the filesystem type, any
need to fsck, instruction set, the distro, and even the type of userspace.
Highly recommended!

> > > > > > +static int
> > > > > > +rcu_torture_shutdown(void *arg)
> > > > > > +{
> > > > > > +	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> > > > > > +	while (ULONG_CMP_LT(jiffies, shutdown_time) &&
> > > > > > +	       !kthread_should_stop()) {
> > > > > > +		if (verbose)
> > > > > > +			printk(KERN_ALERT "%s" TORTURE_FLAG
> > > > > > +			       "rcu_torture_shutdown task: %lu "
> > > > > > +			       "jiffies remaining\n",
> > > > > > +			       torture_type, shutdown_time - jiffies);
> > > > > > +		schedule_timeout_interruptible(HZ);
> > > > > > +	}
> > > > > 
> > > > > Any particular reason to wake up once a second here?  If !verbose, this could just
> > > > > sleep until shutdown time.  (And does the verbose output really help
> > > > > here, given printk timestamps?)
> > > > 
> > > > It actually did help me find a bug where it was failing to shut down.
> > > > I could use different code paths, but that would defeat the debugging.
> > > > 
> > > > So I increased the sleep time to 30 seconds.  Fair enough?
> > > 
> > > Well, now that you've debugged rcutorture's shutdown routine, would it
> > > suffice to have a printk when you actually go to shut down, without
> > > waking up for previous printks when not shutting down yet?
> > > 
> > > (The poll time doesn't really matter, and sleeping for 30 seconds before
> > > checking the time means you might overshoot by up to 30 seconds.  I'd
> > > like to avoid polling to begin with when you know exactly how long you
> > > need to sleep.)
> > 
> > Indeed, good points!  But please see below for what this function turns
> > into when taking that approach.
> 
> See below for responses; that version seems like an improvement, though
> it could still improve further.
> 
> > rcu_torture_shutdown(void *arg)
> > {
> > 	long delta;
> > 	unsigned long jiffies_snap;
> > 
> > 	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> > 	jiffies_snap = ACCESS_ONCE(jiffies);
> 
> Why do you need to snapshot jiffies in this version but not in the
> version you originally posted?

Because in the original, the maximum error was one second, which was
not worth worrying about.

> > 	while (ULONG_CMP_LT(jiffies_snap, shutdown_time) &&
> > 	       !kthread_should_stop()) {
> > 		delta = shutdown_time - jiffies_snap;
> > 		if (verbose)
> > 			printk(KERN_ALERT "%s" TORTURE_FLAG
> > 			       "rcu_torture_shutdown task: %lu "
> > 			       "jiffies remaining\n",
> > 			       torture_type, delta);
> 
> I suggested dropping this print entirely; under normal circumstances it
> should never print.  It will only print if
> schedule_timeout_interruptible wakes up spuriously.

OK, I can qualify with a firsttime local variable.

> > 		schedule_timeout_interruptible(delta);
> > 		jiffies_snap = ACCESS_ONCE(jiffies);
> > 	}
> 
> Any reason this entire loop body couldn't just become
> msleep_interruptible()?

Aha!!!  Because then it won't break out of the loop if someone does
a rmmod of rcutorture.  Which will cause the rmmod to hang until
the thing decides that it is time to shut down the system.  Which
is why I need to do the sleep in smallish pieces -- I cannot sleep
longer than I would be comfortable delaying the rmmod.

Which is why I think I need to revert back to the old version that
did the schedule_timeout_interruptible(1).

> > 	if (ULONG_CMP_LT(jiffies_snap, shutdown_time)) {
> > 		VERBOSE_PRINTK_STRING("rcu_torture_shutdown task stopping");
> > 		return 0;
> > 	}
> 
> Writing this as "if (kthread_should_stop())" seems clearer.

I don't have any real preference here, but as long as I need to
back out the earlier changes, I might as well make this one.  ;-)

							Thanx, Paul

Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability

[Josh Triplett]

On Wed, Nov 16, 2011 at 03:43:58PM -0800, Paul E. McKenney wrote:
> On Wed, Nov 16, 2011 at 02:58:56PM -0800, Josh Triplett wrote:
> > On Wed, Nov 16, 2011 at 02:44:47PM -0800, Paul E. McKenney wrote:
> > > On Wed, Nov 16, 2011 at 02:15:45PM -0800, Josh Triplett wrote:
> > > > On Wed, Nov 16, 2011 at 12:32:26PM -0800, Paul E. McKenney wrote:
> > > > > On Tue, Nov 15, 2011 at 01:46:15PM -0800, Josh Triplett wrote:
> > > > > > On Tue, Nov 15, 2011 at 12:27:58PM -0800, Paul E. McKenney wrote:
> > > > > > > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > > > 
> > > > > > > Although it is easy to run rcutorture tests under KVM, there is currently
> > > > > > > no nice way to run such a test for a fixed time period, collect all of
> > > > > > > the rcutorture data, and then shut the system down cleanly.  This commit
> > > > > > > therefore adds an rcutorture module parameter named "shutdown_secs" that
> > > > > > > specified the run duration in seconds, after which rcutorture terminates
> > > > > > > the test and powers the system down.  The default value for "shutdown_secs"
> > > > > > > is zero, which disables shutdown.
> > > > > > > 
> > > > > > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > > 
> > > > > > >From your recent post on this, I thought you found a solution through
> > > > > > the init= parameter, which seems preferable.
> > > > > 
> > > > > For some things, the init= parameter does work great.  I do intend to
> > > > > use it when collecting event-tracing and debugfs data, for example.
> > > > > 
> > > > > However, there is still a need for RCU torture testing that will operate
> > > > > correctly regardless of how userspace is set up.  That, and there are
> > > > > quite a few different kernel test setup, each with their own peculiar
> > > > > capabilities and limitations.  So what happened was that before people
> > > > > suggested the init= approach, I implemented enough of the in-kernel
> > > > > approach to appreciate how much it simplifies life for the common case of
> > > > > "just torture-test RCU".  As in I should have done this long ago.
> > > > 
> > > > Seems like it would work just as easily to point init at a statically
> > > > linked C program which just sleeps for a fixed time and then shuts down.
> > > > However, given the special-purpose nature of rcutorture, I won't
> > > > complain that strongly.
> > > 
> > > I did consider a statically linked C program, but that can introduce the
> > > need for cross-compilation into situations that do not otherwise need it.
> > 
> > Wouldn't you need to cross-compile the kernel anyway in such situations?
> 
> Not necessarily, consider for example ABAT.  (IBM-specific test setup
> for those unfamiliar with it -- related to autotest.)

Which already handles compiling a kernel for you; ABAT just doesn't make
it as easy to compile userspace programs as it does for kernels. :)

> I suspect that the only way for you to be convinced is for you to write
> a script that takes your preferred approach for injecting a test into
> (say) a KVM instance.

Done and attached.

> Then compare that script to adding a few parameters to the boot line,
> namely: "rcutorture.stat_interval=15 rcutorture.shutdown_secs=3600
> rcutorture.rcutorture_runnable=1".  ;-)

I actually think stat_interval makes perfect sense, as does runnable.

> > > rcu_torture_shutdown(void *arg)
> > > {
> > > 	long delta;
> > > 	unsigned long jiffies_snap;
> > > 
> > > 	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> > > 	jiffies_snap = ACCESS_ONCE(jiffies);
> > 
> > Why do you need to snapshot jiffies in this version but not in the
> > version you originally posted?
> 
> Because in the original, the maximum error was one second, which was
> not worth worrying about.

The original shouldn't have an error either.  If something incorrectly
caches jiffies, either version would sleep forever, not just for an
extra second.

> > > 	while (ULONG_CMP_LT(jiffies_snap, shutdown_time) &&
> > > 	       !kthread_should_stop()) {
> > > 		delta = shutdown_time - jiffies_snap;
> > > 		if (verbose)
> > > 			printk(KERN_ALERT "%s" TORTURE_FLAG
> > > 			       "rcu_torture_shutdown task: %lu "
> > > 			       "jiffies remaining\n",
> > > 			       torture_type, delta);
> > 
> > I suggested dropping this print entirely; under normal circumstances it
> > should never print.  It will only print if
> > schedule_timeout_interruptible wakes up spuriously.
> 
> OK, I can qualify with a firsttime local variable.

Oh, i see; it does print the very first time through.  In that case, you
could move the print out of the loop entirely, rather than using a
"first time" flag.

> > > 		schedule_timeout_interruptible(delta);
> > > 		jiffies_snap = ACCESS_ONCE(jiffies);
> > > 	}
> > 
> > Any reason this entire loop body couldn't just become
> > msleep_interruptible()?
> 
> Aha!!!  Because then it won't break out of the loop if someone does
> a rmmod of rcutorture.  Which will cause the rmmod to hang until
> the thing decides that it is time to shut down the system.  Which
> is why I need to do the sleep in smallish pieces -- I cannot sleep
> longer than I would be comfortable delaying the rmmod.
> 
> Which is why I think I need to revert back to the old version that
> did the schedule_timeout_interruptible(1).

Does kthread_stop not interrupt an interruptible kthread?  As far as I
can tell, rmmod of rcutorture currently finishes immediately, rather
than after all the one-second sleeps finish, which suggests that it
wakes up the threads in question.

- Josh Triplett

Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 5713 bytes --]

[Resending with attachment this time.]

On Wed, Nov 16, 2011 at 03:43:58PM -0800, Paul E. McKenney wrote:
> On Wed, Nov 16, 2011 at 02:58:56PM -0800, Josh Triplett wrote:
> > On Wed, Nov 16, 2011 at 02:44:47PM -0800, Paul E. McKenney wrote:
> > > On Wed, Nov 16, 2011 at 02:15:45PM -0800, Josh Triplett wrote:
> > > > On Wed, Nov 16, 2011 at 12:32:26PM -0800, Paul E. McKenney wrote:
> > > > > On Tue, Nov 15, 2011 at 01:46:15PM -0800, Josh Triplett wrote:
> > > > > > On Tue, Nov 15, 2011 at 12:27:58PM -0800, Paul E. McKenney wrote:
> > > > > > > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > > > 
> > > > > > > Although it is easy to run rcutorture tests under KVM, there is currently
> > > > > > > no nice way to run such a test for a fixed time period, collect all of
> > > > > > > the rcutorture data, and then shut the system down cleanly.  This commit
> > > > > > > therefore adds an rcutorture module parameter named "shutdown_secs" that
> > > > > > > specified the run duration in seconds, after which rcutorture terminates
> > > > > > > the test and powers the system down.  The default value for "shutdown_secs"
> > > > > > > is zero, which disables shutdown.
> > > > > > > 
> > > > > > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > > 
> > > > > > >From your recent post on this, I thought you found a solution through
> > > > > > the init= parameter, which seems preferable.
> > > > > 
> > > > > For some things, the init= parameter does work great.  I do intend to
> > > > > use it when collecting event-tracing and debugfs data, for example.
> > > > > 
> > > > > However, there is still a need for RCU torture testing that will operate
> > > > > correctly regardless of how userspace is set up.  That, and there are
> > > > > quite a few different kernel test setup, each with their own peculiar
> > > > > capabilities and limitations.  So what happened was that before people
> > > > > suggested the init= approach, I implemented enough of the in-kernel
> > > > > approach to appreciate how much it simplifies life for the common case of
> > > > > "just torture-test RCU".  As in I should have done this long ago.
> > > > 
> > > > Seems like it would work just as easily to point init at a statically
> > > > linked C program which just sleeps for a fixed time and then shuts down.
> > > > However, given the special-purpose nature of rcutorture, I won't
> > > > complain that strongly.
> > > 
> > > I did consider a statically linked C program, but that can introduce the
> > > need for cross-compilation into situations that do not otherwise need it.
> > 
> > Wouldn't you need to cross-compile the kernel anyway in such situations?
> 
> Not necessarily, consider for example ABAT.  (IBM-specific test setup
> for those unfamiliar with it -- related to autotest.)

Which already handles compiling a kernel for you; ABAT just doesn't make
it as easy to compile userspace programs as it does for kernels. :)

> I suspect that the only way for you to be convinced is for you to write
> a script that takes your preferred approach for injecting a test into
> (say) a KVM instance.

Done and attached.

> Then compare that script to adding a few parameters to the boot line,
> namely: "rcutorture.stat_interval=15 rcutorture.shutdown_secs=3600
> rcutorture.rcutorture_runnable=1".  ;-)

I actually think stat_interval makes perfect sense, as does runnable.

> > > rcu_torture_shutdown(void *arg)
> > > {
> > > 	long delta;
> > > 	unsigned long jiffies_snap;
> > > 
> > > 	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> > > 	jiffies_snap = ACCESS_ONCE(jiffies);
> > 
> > Why do you need to snapshot jiffies in this version but not in the
> > version you originally posted?
> 
> Because in the original, the maximum error was one second, which was
> not worth worrying about.

The original shouldn't have an error either.  If something incorrectly
caches jiffies, either version would sleep forever, not just for an
extra second.

> > > 	while (ULONG_CMP_LT(jiffies_snap, shutdown_time) &&
> > > 	       !kthread_should_stop()) {
> > > 		delta = shutdown_time - jiffies_snap;
> > > 		if (verbose)
> > > 			printk(KERN_ALERT "%s" TORTURE_FLAG
> > > 			       "rcu_torture_shutdown task: %lu "
> > > 			       "jiffies remaining\n",
> > > 			       torture_type, delta);
> > 
> > I suggested dropping this print entirely; under normal circumstances it
> > should never print.  It will only print if
> > schedule_timeout_interruptible wakes up spuriously.
> 
> OK, I can qualify with a firsttime local variable.

Oh, i see; it does print the very first time through.  In that case, you
could move the print out of the loop entirely, rather than using a
"first time" flag.

> > > 		schedule_timeout_interruptible(delta);
> > > 		jiffies_snap = ACCESS_ONCE(jiffies);
> > > 	}
> > 
> > Any reason this entire loop body couldn't just become
> > msleep_interruptible()?
> 
> Aha!!!  Because then it won't break out of the loop if someone does
> a rmmod of rcutorture.  Which will cause the rmmod to hang until
> the thing decides that it is time to shut down the system.  Which
> is why I need to do the sleep in smallish pieces -- I cannot sleep
> longer than I would be comfortable delaying the rmmod.
> 
> Which is why I think I need to revert back to the old version that
> did the schedule_timeout_interruptible(1).

Does kthread_stop not interrupt an interruptible kthread?  As far as I
can tell, rmmod of rcutorture currently finishes immediately, rather
than after all the one-second sleeps finish, which suggests that it
wakes up the threads in question.

- Josh Triplett

[-- Attachment #2: sleep-shutdown.tar.gz --]
[-- Type: application/octet-stream, Size: 596 bytes --]

Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability

[Paul E. McKenney]

On Wed, Nov 16, 2011 at 04:49:23PM -0800, Josh Triplett wrote:
> [Resending with attachment this time.]
> 
> On Wed, Nov 16, 2011 at 03:43:58PM -0800, Paul E. McKenney wrote:
> > On Wed, Nov 16, 2011 at 02:58:56PM -0800, Josh Triplett wrote:
> > > On Wed, Nov 16, 2011 at 02:44:47PM -0800, Paul E. McKenney wrote:
> > > > On Wed, Nov 16, 2011 at 02:15:45PM -0800, Josh Triplett wrote:
> > > > > On Wed, Nov 16, 2011 at 12:32:26PM -0800, Paul E. McKenney wrote:
> > > > > > On Tue, Nov 15, 2011 at 01:46:15PM -0800, Josh Triplett wrote:
> > > > > > > On Tue, Nov 15, 2011 at 12:27:58PM -0800, Paul E. McKenney wrote:
> > > > > > > > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > > > > 
> > > > > > > > Although it is easy to run rcutorture tests under KVM, there is currently
> > > > > > > > no nice way to run such a test for a fixed time period, collect all of
> > > > > > > > the rcutorture data, and then shut the system down cleanly.  This commit
> > > > > > > > therefore adds an rcutorture module parameter named "shutdown_secs" that
> > > > > > > > specified the run duration in seconds, after which rcutorture terminates
> > > > > > > > the test and powers the system down.  The default value for "shutdown_secs"
> > > > > > > > is zero, which disables shutdown.
> > > > > > > > 
> > > > > > > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > > > 
> > > > > > > >From your recent post on this, I thought you found a solution through
> > > > > > > the init= parameter, which seems preferable.
> > > > > > 
> > > > > > For some things, the init= parameter does work great.  I do intend to
> > > > > > use it when collecting event-tracing and debugfs data, for example.
> > > > > > 
> > > > > > However, there is still a need for RCU torture testing that will operate
> > > > > > correctly regardless of how userspace is set up.  That, and there are
> > > > > > quite a few different kernel test setup, each with their own peculiar
> > > > > > capabilities and limitations.  So what happened was that before people
> > > > > > suggested the init= approach, I implemented enough of the in-kernel
> > > > > > approach to appreciate how much it simplifies life for the common case of
> > > > > > "just torture-test RCU".  As in I should have done this long ago.
> > > > > 
> > > > > Seems like it would work just as easily to point init at a statically
> > > > > linked C program which just sleeps for a fixed time and then shuts down.
> > > > > However, given the special-purpose nature of rcutorture, I won't
> > > > > complain that strongly.
> > > > 
> > > > I did consider a statically linked C program, but that can introduce the
> > > > need for cross-compilation into situations that do not otherwise need it.
> > > 
> > > Wouldn't you need to cross-compile the kernel anyway in such situations?
> > 
> > Not necessarily, consider for example ABAT.  (IBM-specific test setup
> > for those unfamiliar with it -- related to autotest.)
> 
> Which already handles compiling a kernel for you; ABAT just doesn't make
> it as easy to compile userspace programs as it does for kernels. :)

Exactly!  ;-)

Between ABAT, its intended replacement, KVM, and who knows what all else,
the thought of having rcutorture control the test duration via system
shutdown is becoming increasingly attractive...

> > I suspect that the only way for you to be convinced is for you to write
> > a script that takes your preferred approach for injecting a test into
> > (say) a KVM instance.
> 
> Done and attached.

Cute trick!

The scripting has to also include getting the test duration into the .c
file and building it, but that isn't too big of a deal.  Give or take
cross-compilation of the sleep-shutdown.c program, that is...  :-/

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

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

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

FWIW, I used the following command:

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

Thoughts?  (Other than that I should re-build the kernel with
CONFIG_INITRAMFS_SOURCE="", which I will try.)

Just so you know, my next step is to make rcutorture orchestrate the
CPU-hotplug operations, if rcutorture is told to do so and if the kernel
is configured to support this.

> > Then compare that script to adding a few parameters to the boot line,
> > namely: "rcutorture.stat_interval=15 rcutorture.shutdown_secs=3600
> > rcutorture.rcutorture_runnable=1".  ;-)
> 
> I actually think stat_interval makes perfect sense, as does runnable.

Indeed, from what I can see, it is hard to have module parameters without
also having the corresponding kernel boot parameters when that module
is built directly into the kernel.  ;-)

> > > > rcu_torture_shutdown(void *arg)
> > > > {
> > > > 	long delta;
> > > > 	unsigned long jiffies_snap;
> > > > 
> > > > 	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> > > > 	jiffies_snap = ACCESS_ONCE(jiffies);
> > > 
> > > Why do you need to snapshot jiffies in this version but not in the
> > > version you originally posted?
> > 
> > Because in the original, the maximum error was one second, which was
> > not worth worrying about.
> 
> The original shouldn't have an error either.  If something incorrectly
> caches jiffies, either version would sleep forever, not just for an
> extra second.

If it cached it from one iteration of the loop to the next, agreed.
But the new version of the code introduces other possible ways for the
compiler to confuse the issue.

> > > > 	while (ULONG_CMP_LT(jiffies_snap, shutdown_time) &&
> > > > 	       !kthread_should_stop()) {
> > > > 		delta = shutdown_time - jiffies_snap;
> > > > 		if (verbose)
> > > > 			printk(KERN_ALERT "%s" TORTURE_FLAG
> > > > 			       "rcu_torture_shutdown task: %lu "
> > > > 			       "jiffies remaining\n",
> > > > 			       torture_type, delta);
> > > 
> > > I suggested dropping this print entirely; under normal circumstances it
> > > should never print.  It will only print if
> > > schedule_timeout_interruptible wakes up spuriously.
> > 
> > OK, I can qualify with a firsttime local variable.
> 
> Oh, i see; it does print the very first time through.  In that case, you
> could move the print out of the loop entirely, rather than using a
> "first time" flag.

I could, but that would prevent me from seeing cases where there are
multiple passes through the loop.

> > > > 		schedule_timeout_interruptible(delta);
> > > > 		jiffies_snap = ACCESS_ONCE(jiffies);
> > > > 	}
> > > 
> > > Any reason this entire loop body couldn't just become
> > > msleep_interruptible()?
> > 
> > Aha!!!  Because then it won't break out of the loop if someone does
> > a rmmod of rcutorture.  Which will cause the rmmod to hang until
> > the thing decides that it is time to shut down the system.  Which
> > is why I need to do the sleep in smallish pieces -- I cannot sleep
> > longer than I would be comfortable delaying the rmmod.
> > 
> > Which is why I think I need to revert back to the old version that
> > did the schedule_timeout_interruptible(1).
> 
> Does kthread_stop not interrupt an interruptible kthread?  As far as I
> can tell, rmmod of rcutorture currently finishes immediately, rather
> than after all the one-second sleeps finish, which suggests that it
> wakes up the threads in question.

OK, it might well.  But even if kthread_stop() does interrupt an
interruptible kthread, I still need to avoid msleep_interruptible(),
right?

							Thanx, Paul

Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability

[Josh Triplett]

On Wed, Nov 16, 2011 at 05:40:57PM -0800, Paul E. McKenney wrote:
> On Wed, Nov 16, 2011 at 04:49:23PM -0800, Josh Triplett wrote:
> > On Wed, Nov 16, 2011 at 03:43:58PM -0800, Paul E. McKenney wrote:
> > > I suspect that the only way for you to be convinced is for you to write
> > > a script that takes your preferred approach for injecting a test into
> > > (say) a KVM instance.
> > 
> > Done and attached.
> 
> Cute trick!
> 
> The scripting has to also include getting the test duration into the .c
> file and building it, but that isn't too big of a deal.  Give or take
> cross-compilation of the sleep-shutdown.c program, that is...  :-/

:)

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

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

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

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

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

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

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

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

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

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

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

On Wed, Nov 16, 2011 at 05:40:57PM -0800, Paul E. McKenney wrote:
> On Wed, Nov 16, 2011 at 04:49:23PM -0800, Josh Triplett wrote:
> > On Wed, Nov 16, 2011 at 03:43:58PM -0800, Paul E. McKenney wrote:
> > > On Wed, Nov 16, 2011 at 02:58:56PM -0800, Josh Triplett wrote:
> > > > On Wed, Nov 16, 2011 at 02:44:47PM -0800, Paul E. McKenney wrote:
> > > > > rcu_torture_shutdown(void *arg)
> > > > > {
> > > > > 	long delta;
> > > > > 	unsigned long jiffies_snap;
> > > > > 
> > > > > 	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> > > > > 	jiffies_snap = ACCESS_ONCE(jiffies);
> > > > 
> > > > Why do you need to snapshot jiffies in this version but not in the
> > > > version you originally posted?
> > > 
> > > Because in the original, the maximum error was one second, which was
> > > not worth worrying about.
> > 
> > The original shouldn't have an error either.  If something incorrectly
> > caches jiffies, either version would sleep forever, not just for an
> > extra second.
> 
> If it cached it from one iteration of the loop to the next, agreed.
> But the new version of the code introduces other possible ways for the
> compiler to confuse the issue.

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

> > > > > 	while (ULONG_CMP_LT(jiffies_snap, shutdown_time) &&
> > > > > 	       !kthread_should_stop()) {
> > > > > 		delta = shutdown_time - jiffies_snap;
> > > > > 		if (verbose)
> > > > > 			printk(KERN_ALERT "%s" TORTURE_FLAG
> > > > > 			       "rcu_torture_shutdown task: %lu "
> > > > > 			       "jiffies remaining\n",
> > > > > 			       torture_type, delta);
> > > > 
> > > > I suggested dropping this print entirely; under normal circumstances it
> > > > should never print.  It will only print if
> > > > schedule_timeout_interruptible wakes up spuriously.
> > > 
> > > OK, I can qualify with a firsttime local variable.
> > 
> > Oh, i see; it does print the very first time through.  In that case, you
> > could move the print out of the loop entirely, rather than using a
> > "first time" flag.
> 
> I could, but that would prevent me from seeing cases where there are
> multiple passes through the loop.

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

> > > > > 		schedule_timeout_interruptible(delta);
> > > > > 		jiffies_snap = ACCESS_ONCE(jiffies);
> > > > > 	}
> > > > 
> > > > Any reason this entire loop body couldn't just become
> > > > msleep_interruptible()?
> > > 
> > > Aha!!!  Because then it won't break out of the loop if someone does
> > > a rmmod of rcutorture.  Which will cause the rmmod to hang until
> > > the thing decides that it is time to shut down the system.  Which
> > > is why I need to do the sleep in smallish pieces -- I cannot sleep
> > > longer than I would be comfortable delaying the rmmod.
> > > 
> > > Which is why I think I need to revert back to the old version that
> > > did the schedule_timeout_interruptible(1).
> > 
> > Does kthread_stop not interrupt an interruptible kthread?  As far as I
> > can tell, rmmod of rcutorture currently finishes immediately, rather
> > than after all the one-second sleeps finish, which suggests that it
> > wakes up the threads in question.
> 
> OK, it might well.  But even if kthread_stop() does interrupt an
> interruptible kthread, I still need to avoid msleep_interruptible(),
> right?

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

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

- Josh Triplett

Re: [PATCH tip/core/rcu 2/9] rcu: Add rcutorture system-shutdown capability

[Paul E. McKenney]

On Thu, Nov 17, 2011 at 03:57:33PM -0800, Josh Triplett wrote:
> On Wed, Nov 16, 2011 at 05:40:57PM -0800, Paul E. McKenney wrote:
> > On Wed, Nov 16, 2011 at 04:49:23PM -0800, Josh Triplett wrote:
> > > On Wed, Nov 16, 2011 at 03:43:58PM -0800, Paul E. McKenney wrote:
> > > > I suspect that the only way for you to be convinced is for you to write
> > > > a script that takes your preferred approach for injecting a test into
> > > > (say) a KVM instance.
> > > 
> > > Done and attached.
> > 
> > Cute trick!
> > 
> > The scripting has to also include getting the test duration into the .c
> > file and building it, but that isn't too big of a deal.  Give or take
> > cross-compilation of the sleep-shutdown.c program, that is...  :-/
> 
> :)
> 
> > However, this approach does cause the rcutorture success/failure message
> > to be lost.  One possible way around this would be to put rcutorture.ko
> > into your initrd, then make the program insmod it, sleep, rmmod it,
> > and then shut down.  But unless I am missing something basic (which is
> > quite possible), just doing the shutdown inside rcutorture is simpler
> > at that point.
> 
> When you build rcutorture into the kernel, the module's exit function
> will never get called at all.  If you want to see the final summary, you
> might want to build in a mechanism for rcutorture to quiesce even when
> built in, and then arrange to run that via a shutdown notifier.  That
> seems like the right way around: you shut down the system, and the
> built-in rcutorture notices and gives a final summary.
> 
> Alternatively, similar to your addition of rcutorture.stat_interval to
> periodically write out a summary, you might consider having rcutorture
> periodically quiesce and write out a PASS/FAIL.

Except that I need my test-analysis scripts to be able to easily
distinguish between a shutdown that rcutorture was OK with and a "rogue"
shutdown that terminated the test prematurely.

> > However, the thought of improving boot speed by confining the kernel to
> > a very simple initrd does sound attractive.
> 
> Definitely.  I frequently use an initrd rather than creating an entire
> filesystem, and the result runs very quickly.  It also proves easier to
> build than a block device.

Faster to build, as well.  ;-)

However, I am not convinced that initrd is generally applicable.  I have
seen kernels that get annoyed if the userspace doesn't take action within
a given period of time.  Such a kernel would not like being handed an
initrd sandbox.

> > Interesting behavior.  I forgot that the bzImage that I had lying around
> > already had an initrd built into it.  The kernel seemed to start up on
> > the built-in initrd, complete with serial output.  Then powered off after
> > about 30 seconds (I did s/3600/30/ in your sleep-shutdown.c), rather than
> > the 120 I had specified to rcutorture (preventing any success/failure
> > message from rcutorture as well).  Two initrds executing in parallel?
> > Hmmm...
> 
> Huh.  Odd.  But no, initramfses don't execute in parallel, or in series
> for that matter.  The kernel will extract its built-in initramfs to the
> "rootfs" tmpfs filesystem, then extract any externally-supplied
> initramfs on top of that, and then run /init in the rootfs filesystem.
> So, the /init from sleep-shutdown would overwrite any /init from your
> built-in initramfs, and most likely nothing else from your built-in
> initramfs had any effect at all.

Quite possibly.

> > FWIW, I used the following command:
> > 
> > kvm -kernel linux-2.6/arch/x86/boot/bzImage -initrd /tmp/sleep-shutdown/sleep-shutdown -serial file:/home/kvm/linux-2.6/console.log -nographic -smp 2 -m 512 -append "noapic selinux=0 console=ttyS0 initcall_debug debug rcutorture.stat_interval=15 rcutorture.shutdown_secs=120 rcutorture.rcutorture_runnable=1"
> 
> Looks reasonable, other than that you don't need
> rcutorture.shutdown_secs if you use sleep-shutdown.  Also, as far as I
> know you shouldn't need noapic with KVM; if you do then you should
> report a KVM bug.  (Or does that represent a scenario or code path you
> wanted to test RCU under?)

Right -- I was setting both to different values in order to make sure
I could tell which was having effect.  In this case, the sleep-shutdown
deadline arrived soonest, so it won.

The "noapic" seemed to be necessary in my initial attempts to test RCU
under qemu -- perhaps not needed for kvm?

> > Thoughts?  (Other than that I should re-build the kernel with
> > CONFIG_INITRAMFS_SOURCE="", which I will try.)
> > 
> > Just so you know, my next step is to make rcutorture orchestrate the
> > CPU-hotplug operations, if rcutorture is told to do so and if the kernel
> > is configured to support this.
> 
> And that really seems easier than just running a simple script from an
> initrd?

Yes, easier and less fragile, especially with respect to automatically
analyzing the test results.

> On Wed, Nov 16, 2011 at 05:40:57PM -0800, Paul E. McKenney wrote:
> > On Wed, Nov 16, 2011 at 04:49:23PM -0800, Josh Triplett wrote:
> > > On Wed, Nov 16, 2011 at 03:43:58PM -0800, Paul E. McKenney wrote:
> > > > On Wed, Nov 16, 2011 at 02:58:56PM -0800, Josh Triplett wrote:
> > > > > On Wed, Nov 16, 2011 at 02:44:47PM -0800, Paul E. McKenney wrote:
> > > > > > rcu_torture_shutdown(void *arg)
> > > > > > {
> > > > > > 	long delta;
> > > > > > 	unsigned long jiffies_snap;
> > > > > > 
> > > > > > 	VERBOSE_PRINTK_STRING("rcu_torture_shutdown task started");
> > > > > > 	jiffies_snap = ACCESS_ONCE(jiffies);
> > > > > 
> > > > > Why do you need to snapshot jiffies in this version but not in the
> > > > > version you originally posted?
> > > > 
> > > > Because in the original, the maximum error was one second, which was
> > > > not worth worrying about.
> > > 
> > > The original shouldn't have an error either.  If something incorrectly
> > > caches jiffies, either version would sleep forever, not just for an
> > > extra second.
> > 
> > If it cached it from one iteration of the loop to the next, agreed.
> > But the new version of the code introduces other possible ways for the
> > compiler to confuse the issue.
> 
> At least in theory, the compiler can't cache the value of jiffies at
> all, since jiffies uses "volatile".  The CPU potentially could, but we
> don't normally expect CPUs to hold onto old cached values indefinitely;
> we tend to count on updates to eventually propagate.

OK, I missed the "volatile" on jiffies.  Bad cscope day, I guess.

> > > > > > 	while (ULONG_CMP_LT(jiffies_snap, shutdown_time) &&
> > > > > > 	       !kthread_should_stop()) {
> > > > > > 		delta = shutdown_time - jiffies_snap;
> > > > > > 		if (verbose)
> > > > > > 			printk(KERN_ALERT "%s" TORTURE_FLAG
> > > > > > 			       "rcu_torture_shutdown task: %lu "
> > > > > > 			       "jiffies remaining\n",
> > > > > > 			       torture_type, delta);
> > > > > 
> > > > > I suggested dropping this print entirely; under normal circumstances it
> > > > > should never print.  It will only print if
> > > > > schedule_timeout_interruptible wakes up spuriously.
> > > > 
> > > > OK, I can qualify with a firsttime local variable.
> > > 
> > > Oh, i see; it does print the very first time through.  In that case, you
> > > could move the print out of the loop entirely, rather than using a
> > > "first time" flag.
> > 
> > I could, but that would prevent me from seeing cases where there are
> > multiple passes through the loop.
> 
> ...which should never happen unless something signals that thread,
> right?  (And normally, that signal will occur as part of kthread_stop,
> which would terminate the loop as well.)

Exactly.  It should never happen, so if I am worried enough about what
rcutorture is doing to specify "verbose" (which I rarely do), then I
want to see it.

> > > > > > 		schedule_timeout_interruptible(delta);
> > > > > > 		jiffies_snap = ACCESS_ONCE(jiffies);
> > > > > > 	}
> > > > > 
> > > > > Any reason this entire loop body couldn't just become
> > > > > msleep_interruptible()?
> > > > 
> > > > Aha!!!  Because then it won't break out of the loop if someone does
> > > > a rmmod of rcutorture.  Which will cause the rmmod to hang until
> > > > the thing decides that it is time to shut down the system.  Which
> > > > is why I need to do the sleep in smallish pieces -- I cannot sleep
> > > > longer than I would be comfortable delaying the rmmod.
> > > > 
> > > > Which is why I think I need to revert back to the old version that
> > > > did the schedule_timeout_interruptible(1).
> > > 
> > > Does kthread_stop not interrupt an interruptible kthread?  As far as I
> > > can tell, rmmod of rcutorture currently finishes immediately, rather
> > > than after all the one-second sleeps finish, which suggests that it
> > > wakes up the threads in question.
> > 
> > OK, it might well.  But even if kthread_stop() does interrupt an
> > interruptible kthread, I still need to avoid msleep_interruptible(),
> > right?
> 
> I don't see any reason you need to avoid it.  As far as I can tell,
> msleep_interruptible should do *exactly* what you want, including
> allowing interruptions by kthread_stop.  You just need something like
> this:
> 
> unsigned int timeout = ...;
> while (timeout && !kthread_should_stop())
>     timeout = msleep_interruptible(timeout);

Color me confused.  Here is msleep_interruptible():

	unsigned long msleep_interruptible(unsigned int msecs)
	{
		unsigned long timeout = msecs_to_jiffies(msecs) + 1;

		while (timeout && !signal_pending(current))
			timeout = schedule_timeout_interruptible(timeout);
		return jiffies_to_msecs(timeout);
	}

And here is signal_pending():

	static inline int signal_pending(struct task_struct *p)
	{
		return unlikely(test_tsk_thread_flag(p,TIF_SIGPENDING));
	}

And finally, here is kthread_stop():

	int kthread_stop(struct task_struct *k)
	{
		struct kthread *kthread;
		int ret;

		trace_sched_kthread_stop(k);
		get_task_struct(k);

		kthread = to_kthread(k);
		barrier(); /* it might have exited */
		if (k->vfork_done != NULL) {
			kthread->should_stop = 1;
			wake_up_process(k);
			wait_for_completion(&kthread->exited);
		}
		ret = k->exit_code;

		put_task_struct(k);
		trace_sched_kthread_stop_ret(ret);

		return ret;
	}

I don't see how kthread_stop() is doing anything to kick
msleep_interruptible() out of its loop.  What am I missing?

							Thanx, Paul

Re: [PATCH tip/core/rcu 9/9] tile: Make tile use the new is_idle_task() API

[Chris Metcalf]

(Narrowing cc's)

On 11/15/2011 3:30 PM, Paul E. McKenney wrote:
> From: Paul E. McKenney <paul.mckenney@linaro.org>
> 
> Change from direct comparison of ->pid with zero to is_idle_task().
> 
> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  arch/tile/mm/fault.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

Acked-by: Chris Metcalf <cmetcalf@tilera.com>

Please cc me on changes to tile architecture files - thanks!
I picked this up while belatedly catching up on LKML.

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

[Paul E. McKenney]

On Wed, Nov 23, 2011 at 12:03:43PM -0500, Chris Metcalf wrote:
> (Narrowing cc's)
> 
> On 11/15/2011 3:30 PM, Paul E. McKenney wrote:
> > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > 
> > Change from direct comparison of ->pid with zero to is_idle_task().
> > 
> > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >  arch/tile/mm/fault.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> Acked-by: Chris Metcalf <cmetcalf@tilera.com>
> 
> 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