Don not IPI offline CPUs, de-emphasize cond_resched_rcu_qs()

Now that cond_resched() also provides RCU quiescent states when needed, it can be used in place of cond_resched_rcu_qs(). This commit therefore makes this change.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Lai Jiangshan <[email protected]>

---
 kernel/workqueue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/workqueue.c +1 -2

index 8fdb710bfdd7..aee7eaab05cb 100644
@@ -2135,7 +2135,7 @@ __acquires(&pool->lock)
         * stop_machine. At the same time, report a quiescent RCU state so
         * the same condition doesn't freeze RCU.
         */
-       cond_resched_rcu_qs();
+       cond_resched();
 
        spin_lock_irq(&pool->lock);
 
--

2.5.2

Quoted 7 lines · Paul E. McKenney

Now that cond_resched() also provides RCU quiescent states when needed, it can be used in place of cond_resched_rcu_qs(). This commit therefore makes this change.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Lai Jiangshan <[email protected]>

Reviewed-by Lai Jiangshan <[email protected]>

Quoted 20 lines

---
 kernel/workqueue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/workqueue.c +1 -2

index 8fdb710bfdd7..aee7eaab05cb 100644
@@ -2135,7 +2135,7 @@ __acquires(&pool->lock)
         * stop_machine. At the same time, report a quiescent RCU state so
         * the same condition doesn't freeze RCU.
         */
-       cond_resched_rcu_qs();
+       cond_resched();
 
        spin_lock_irq(&pool->lock);
 
--

2.5.2

Now that cond_resched() also provides RCU quiescent states when needed, it can be used in place of cond_resched_rcu_qs(). This commit therefore makes this change.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Ingo Molnar <[email protected]>

---
 kernel/trace/trace_benchmark.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/trace/trace_benchmark.c +1 -2

index 79f838a75077..22fee766081b 100644
@@ -165,7 +165,7 @@ static int benchmark_event_kthread(void *arg)
 		 * this thread will never voluntarily schedule which would
 		 * block synchronize_rcu_tasks() indefinitely.
 		 */
-		cond_resched_rcu_qs();
+		cond_resched();
 	}
 
 	return 0;
-

Quoted 3 lines · On Fri, 1 Dec 2017 11:21:40 -0800 "Paul E. McKenney"…

Now that cond_resched() also provides RCU quiescent states when needed, it can be used in place of cond_resched_rcu_qs(). This commit therefore makes this change.

Are you sure this is true?

I just bisected a lock up on my machine down to this commit.

With CONFIG_TRACEPOINT_BENCHMARK=y

# cd linux.git/tools/testing/selftests/ftrace/
# ./ftracetest test.d/ftrace/func_traceonoff_triggers.tc

Locks up with a backtrace of:

[  614.186509] INFO: rcu_tasks detected stalls on tasks:

Quoted 8 lines · Steven Rostedt

Quoted 3 lines · On Fri, 1 Dec 2017 11:21:40 -0800 "Paul E. McKenney"…

Now that cond_resched() also provides RCU quiescent states when needed, it can be used in place of cond_resched_rcu_qs(). This commit therefore makes this change.

Are you sure this is true?

Up to a point. If a given CPU has been blocking an RCU grace period for long enough, that CPU's rcu_dynticks.rcu_need_heavy_qs will be set, and then the next cond_resched() will be treated as a cond_resched_rcu_qs().

However, to your point, if there is no grace period in progress or if the current grace period is not waiting on the CPU in question or if the grace-period kthread is starved of CPU, then cond_resched() has no effect on RCU. Unless of course it results in a context switch.

Quoted 10 lines

I just bisected a lock up on my machine down to this commit.

With CONFIG_TRACEPOINT_BENCHMARK=y

# cd linux.git/tools/testing/selftests/ftrace/
# ./ftracetest test.d/ftrace/func_traceonoff_triggers.tc

Locks up with a backtrace of:

[  614.186509] INFO: rcu_tasks detected stalls on tasks:

Ah, but this is RCU-tasks! Which never sets rcu_dynticks.rcu_need_heavy_qs, thus needing a real context switch.

Hey, when you said that synchronize_rcu_tasks() could take a very long time, I took you at your word! ;-)

Does the following (untested, probably does not even build) patch make cond_resched() take a more peremptory approach to RCU-tasks?

Quoted 30 lines · Paul E. McKenney

Quoted 8 lines · Steven Rostedt

Quoted 3 lines · On Fri, 1 Dec 2017 11:21:40 -0800 "Paul E. McKenney"…

Now that cond_resched() also provides RCU quiescent states when needed, it can be used in place of cond_resched_rcu_qs(). This commit therefore makes this change.

Are you sure this is true?

Up to a point. If a given CPU has been blocking an RCU grace period for long enough, that CPU's rcu_dynticks.rcu_need_heavy_qs will be set, and then the next cond_resched() will be treated as a cond_resched_rcu_qs().

However, to your point, if there is no grace period in progress or if the current grace period is not waiting on the CPU in question or if the grace-period kthread is starved of CPU, then cond_resched() has no effect on RCU. Unless of course it results in a context switch.

Quoted 10 lines

I just bisected a lock up on my machine down to this commit.

With CONFIG_TRACEPOINT_BENCHMARK=y

# cd linux.git/tools/testing/selftests/ftrace/
# ./ftracetest test.d/ftrace/func_traceonoff_triggers.tc

Locks up with a backtrace of:

[  614.186509] INFO: rcu_tasks detected stalls on tasks:

Quoted 8 lines

Ah, but this is RCU-tasks! Which never sets rcu_dynticks.rcu_need_heavy_qs, thus needing a real context switch.

Hey, when you said that synchronize_rcu_tasks() could take a very long time, I took you at your word! ;-)

Does the following (untested, probably does not even build) patch make cond_resched() take a more peremptory approach to RCU-tasks?

And probably not. You are probably running CONFIG_PREEMPT=y (otherwise RCU-tasks is trivial), so cond_resched() is a complete no-op:

static inline int _cond_resched(void) { return 0; }

I could make this call rcu_all_qs(), but I would not expect Peter Zijlstra to be at all happy with that sort of change.

And the people who asked for the cond_resched() work probably aren't going to be happy with the resumed proliferation of cond_resched_rcu_qs().

Hmmm... Grasping at straws... Could we make cond_resched() be something like a tracepoint and instrument them with cond_resched_rcu_qs() if the current RCU-tasks grace period ran for more that (say) a minute of its ten-minute stall-warning span?

Quoted 4 lines

Hmmm... Grasping at straws... Could we make cond_resched() be something like a tracepoint and instrument them with cond_resched_rcu_qs() if the current RCU-tasks grace period ran for more that (say) a minute of its ten-minute stall-warning span?

On the other hand, you noted in your other email that the tracepoint benchmark should not be enabled on production systems. So how about the following (again untested) patch? The "defined(CONFIG_TASKS_RCU)" might need to change, especially if RCU-tasks is used in production kernels, but perhaps a starting point.

Thanx, Paul

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

include/linux/sched.h +0 -0

index b161ef8a902e..316c29c5e506 100644
@@ -1589,6 +1589,12 @@ static inline int test_tsk_need_resched(struct task_struct *tsk)
  */
 #ifndef CONFIG_PREEMPT

Quoted 10 lines · On Sun, 25 Feb 2018 10:39:44 -0800 "Paul E. McKenney"…

Quoted 4 lines

Hmmm... Grasping at straws... Could we make cond_resched() be something like a tracepoint and instrument them with cond_resched_rcu_qs() if the current RCU-tasks grace period ran for more that (say) a minute of its ten-minute stall-warning span?

On the other hand, you noted in your other email that the tracepoint benchmark should not be enabled on production systems. So how about the following (again untested) patch? The "defined(CONFIG_TASKS_RCU)" might need to change, especially if RCU-tasks is used in production kernels, but perhaps a starting point.

RCU tasks are used in production systems if PREEMPT is enabled (it allows for optimizations with ftrace, perf and kprobes).

But the tracepoint is not used.

Quoted 15 lines

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

include/linux/sched.h +5 -0

index b161ef8a902e..316c29c5e506 100644
@@ -1589,6 +1589,12 @@ static inline int test_tsk_need_resched(struct task_struct *tsk)
  */
 #ifndef CONFIG_PREEMPT
 extern int _cond_resched(void);
+#elif defined(CONFIG_TASKS_RCU)
+static inline int _cond_resched(void)
+{
+	rcu_note_voluntary_context_switch(current);
+	return 0;

include/linux/sched.h static inline int test_tsk_need_resched(struct task_struct *tsk) · line 1600

Quoted 4 lines

include/linux/sched.h +1 -0

+}
 #else
 static inline int _cond_resched(void) { return 0; }
 #endif

This does work, but so does the below, without causing cond_resched() from being something other than a nop of CONFIG_PREEMPT.

-- Steve

kernel/trace/trace_benchmark.c +1 -0

index 22fee766081b..82d83bb4874b 100644
@@ -166,6 +166,7 @@ static int benchmark_event_kthread(void *arg)
 		 * block synchronize_rcu_tasks() indefinitely.
 		 */
 		cond_resched();
+		rcu_note_voluntary_context_switch(current);
 	}
 
 	return 0;

Quoted 11 lines

Quoted 11 lines

include/linux/sched.h +5 -0

@@ -1589,6 +1589,12 @@ static inline int test_tsk_need_resched(struct task_struct *tsk)
  */
 #ifndef CONFIG_PREEMPT
 extern int _cond_resched(void);
+#elif defined(CONFIG_TASKS_RCU)
+static inline int _cond_resched(void)
+{
+	rcu_note_voluntary_context_switch(current);
+	return 0;

Quoted 8 lines

include/linux/sched.h static inline int test_tsk_need_resched(struct task_struct *tsk) · line 1600

Quoted 4 lines

include/linux/sched.h +1 -0

+}
 #else
 static inline int _cond_resched(void) { return 0; }
 #endif  

This does work, but so does the below, without causing cond_resched() from being something other than a nop of CONFIG_PREEMPT.

True, but based on the cond_resched_rcu_qs() experience, I bet that trace_benchmark.c won't be the only place needing help.

Quoted 20 lines · On Tue, 27 Feb 2018 07:36:46 -0800 "Paul E. McKenney"…

Quoted 19 lines

Quoted 5 lines

include/linux/sched.h +0 -0

@@ -1589,6 +1589,12 @@ static inline int test_tsk_need_resched(struct task_struct *tsk)
  */
 #ifndef CONFIG_PREEMPT

include/linux/sched.h static inline int test_tsk_need_resched(struct task_struct *tsk) · line 1600

Quoted 10 lines

include/linux/sched.h +6 -0

 extern int _cond_resched(void);
+#elif defined(CONFIG_TASKS_RCU)
+static inline int _cond_resched(void)
+{
+	rcu_note_voluntary_context_switch(current);
+	return 0;
+}
 #else
 static inline int _cond_resched(void) { return 0; }
 #endif  

This does work, but so does the below, without causing cond_resched() from being something other than a nop of CONFIG_PREEMPT.

Quoted 2 lines

True, but based on the cond_resched_rcu_qs() experience, I bet that trace_benchmark.c won't be the only place needing help.

Perhaps, still think this is a special case. That said, perhaps cond_resched isn't done in critical locations as it's a place that is explicitly stating that it's OK to schedule.

Quoted 3 lines

Perhaps, still think this is a special case. That said, perhaps cond_resched isn't done in critical locations as it's a place that is explicitly stating that it's OK to schedule.

Building on your second sentence, when you are running a non-production stress test, adding an extra function call and conditional branch to cond_resched() should not be a problem.

So how about the (still untested) patch below?

Thanx, Paul

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

commit e9a6ea9fc2542459f9a63cf2b3a0264d09fbc266

Author: Paul E. McKenney <[email protected]>
Date:   Sun Feb 25 10:40:44 2018 -0800

EXP sched: Make non-production PREEMPT cond_resched() help Tasks RCU

In CONFIG_PREEMPT=y kernels, cond_resched() is a complete no-op, and thus cannot help advance Tasks-RCU grace periods. However, such grace periods are only an issue in non-production benchmarking runs of the Linux kernel. This commit therefore makes cond_resched() invoke rcu_note_voluntary_context_switch() for kernels implementing Tasks RCU even in CONFIG_PREEMPT=y kernels.

Reported-by Steven Rostedt <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

include/linux/sched.h +0 -0

index b161ef8a902e..970dadefb86f 100644

include/linux/sched.h +2 -0

+	return 0;
+}
 #else
 static inline int _cond_resched(void) { return 0; }
 #endif

Quoted 40 lines · On Wed, 28 Feb 2018 17:21:44 -0800 "Paul E. McKenney"…

Quoted 3 lines

Perhaps, still think this is a special case. That said, perhaps cond_resched isn't done in critical locations as it's a place that is explicitly stating that it's OK to schedule.

Building on your second sentence, when you are running a non-production stress test, adding an extra function call and conditional branch to cond_resched() should not be a problem.

So how about the (still untested) patch below?

Thanx, Paul

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

commit e9a6ea9fc2542459f9a63cf2b3a0264d09fbc266

Author: Paul E. McKenney <[email protected]>
Date:   Sun Feb 25 10:40:44 2018 -0800

EXP sched: Make non-production PREEMPT cond_resched() help Tasks RCU

In CONFIG_PREEMPT=y kernels, cond_resched() is a complete no-op, and thus cannot help advance Tasks-RCU grace periods. However, such grace periods are only an issue in non-production benchmarking runs of the Linux kernel. This commit therefore makes cond_resched() invoke rcu_note_voluntary_context_switch() for kernels implementing Tasks RCU even in CONFIG_PREEMPT=y kernels.

Reported-by Steven Rostedt <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

include/linux/sched.h +2 -0

index b161ef8a902e..970dadefb86f 100644
@@ -1589,6 +1589,12 @@ static inline int test_tsk_need_resched(struct task_struct *tsk)
  */
 #ifndef CONFIG_PREEMPT
 extern int _cond_resched(void);
+#elif defined(CONFIG_TRACEPOINT_BENCHMARK)
+static inline int _cond_resched(void)

include/linux/sched.h static inline int test_tsk_need_resched(struct task_struct *tsk) · line 1595

Quoted 2 lines

include/linux/sched.h +2 -0

+{
+	rcu_note_voluntary_context_switch(current);

The thing I hate about this is that it is invasive to code outside of the tracepoint benchmark. Why do the rcu_note_voluntary_context_switch here and not in the tracepoint code? Seems odd to have it called everywhere in the kernel when it is only needed by the benchmark tracepoint code.

-- Steve

Quoted 5 lines

include/linux/sched.h +2 -0

+	return 0;
+}
 #else
 static inline int _cond_resched(void) { return 0; }
 #endif

So how about I rename cond_resched_rcu_qs() to cond_resched_tasks_rcu_qs(), which at least gives a hint as to where it needs to be used?

Would that work for you?

Quoted 4 lines · On Thu, 1 Mar 2018 12:48:58 -0800 "Paul E. McKenney"…

So how about I rename cond_resched_rcu_qs() to cond_resched_tasks_rcu_qs(), which at least gives a hint as to where it needs to be used?

Would that work for you?

Yes, definitely!

And probably not. You are probably running CONFIG_PREEMPT=y (otherwise RCU-tasks is trivial), so cond_resched() is a complete no-op:

static inline int _cond_resched(void) { return 0; }

I could make this call rcu_all_qs(), but I would not expect Peter Zijlstra to be at all happy with that sort of change.

And the people who asked for the cond_resched() work probably aren't going to be happy with the resumed proliferation of cond_resched_rcu_qs().

Hmmm... Grasping at straws... Could we make cond_resched() be something like a tracepoint and instrument them with cond_resched_rcu_qs() if the current RCU-tasks grace period ran for more that (say) a minute of its ten-minute stall-warning span?

Quoted 8 lines · On Sun, 25 Feb 2018 10:17:30 -0800 "Paul E. McKenney"…

And probably not. You are probably running CONFIG_PREEMPT=y (otherwise RCU-tasks is trivial), so cond_resched() is a complete no-op:

static inline int _cond_resched(void) { return 0; }

I could make this call rcu_all_qs(), but I would not expect Peter Zijlstra to be at all happy with that sort of change.

Quoted 8 lines

And the people who asked for the cond_resched() work probably aren't going to be happy with the resumed proliferation of cond_resched_rcu_qs().

Hmmm... Grasping at straws... Could we make cond_resched() be something like a tracepoint and instrument them with cond_resched_rcu_qs() if the current RCU-tasks grace period ran for more that (say) a minute of its ten-minute stall-warning span?

Instead of monkeying with cond_resched(), since this is "special" code, why don't I just have that code call it directly?

	cond_resched();
	rcu_note_voluntary_context_switch(current);

If cond_resched() returns false, then it has already invoked rcu_all_qs(). This is also invoked (now redundantly) by rcu_note_voluntary_context_switch(). This commit therefore changes cond_resched_rcu_qs() to invoke rcu_note_voluntary_context_switch_lite() instead of rcu_note_voluntary_context_switch() to avoid the redundant invocation of rcu_all_qs().

Signed-off-by Paul E. McKenney <[email protected]>

---
 include/linux/rcupdate.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/rcupdate.h +0 -0

index a6ddc42f87a5..7bd8b5a6db10 100644

Quoted 12 lines · Paul E. McKenney

If cond_resched() returns false, then it has already invoked rcu_all_qs(). This is also invoked (now redundantly) by rcu_note_voluntary_context_switch(). This commit therefore changes cond_resched_rcu_qs() to invoke rcu_note_voluntary_context_switch_lite() instead of rcu_note_voluntary_context_switch() to avoid the redundant invocation of rcu_all_qs().

Signed-off-by Paul E. McKenney <[email protected]>

---
 include/linux/rcupdate.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/rcupdate.h static inline void exit_tasks_rcu_finish(void) { } · line 202

Quoted 12 lines

include/linux/rcupdate.h +1 -1

index a6ddc42f87a5..7bd8b5a6db10 100644
@@ -197,7 +197,7 @@ static inline void exit_tasks_rcu_finish(void) { }
 #define cond_resched_rcu_qs() \
 do { \
 	if (!cond_resched()) \
-		rcu_note_voluntary_context_switch(current); \
+		rcu_note_voluntary_context_switch_lite(current); \
 } while (0)
 

Maybe I'm confused, but why are we keeping cond_resched_rcu_qs() around at all?

Because there are a few key places within RCU and rcutorture that need it. Without it, there are scenarios where the new cond_resched() never gets activated, and thus doesn't take effect.

The key point is that with this series in place, it should not be necessary to use cond_resched_rcu_qs() outside of kernel/rcu and kernel/torture.c. Which is a valuable step forward, right?

Because there are a few key places within RCU and rcutorture that need it. Without it, there are scenarios where the new cond_resched() never gets activated, and thus doesn't take effect.

The key point is that with this series in place, it should not be necessary to use cond_resched_rcu_qs() outside of kernel/rcu and kernel/torture.c. Which is a valuable step forward, right?

Quoted 4 lines · On Sat, 2 Dec 2017 04:22:20 -0800 "Paul E. McKenney"…

Because there are a few key places within RCU and rcutorture that need it. Without it, there are scenarios where the new cond_resched() never gets activated, and thus doesn't take effect.

Quoted 3 lines

The key point is that with this series in place, it should not be necessary to use cond_resched_rcu_qs() outside of kernel/rcu and kernel/torture.c. Which is a valuable step forward, right?

I'm guessing the tracepoint benchmark is another situation. It's only existence is to benchmark tracepoints and should not be enabled on any production system. Thus, I think reverting patch 6 (the one removing it from the benchmark code) is the proper solution.