General fixes

There is some confusion as to which of cond_resched() or cond_resched_rcu_qs() should be added to long in-kernel loops. This commit therefore eliminates the decision by adding RCU quiescent states to cond_resched(). This commit also simplifies the code that used to interact with cond_resched_rcu_qs(), and that now interacts with cond_resched(), to reduce its overhead. This reduction is necessary to allow the heavier-weight cond_resched_rcu_qs() mechanism to be invoked everywhere that cond_resched() is invoked.

Part of that reduction in overhead converts the jiffies_till_sched_qs kernel parameter to read-only at runtime, thus eliminating the need for bounds checking.

Reported-by Michal Hocko <[email protected]>

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

Cc Peter Zijlstra <[email protected]>

[ paulmck: Tuning for performance issues reported by 0day Test Robot. ]

include/linux/sched.h +2 -1

index 8337e2db0bb2..d2f291a3a44a 100644
@@ -1522,10 +1522,11 @@ static inline int test_tsk_need_resched(struct task_struct *tsk)
  * cond_resched_lock() will drop the spinlock before scheduling,
  * cond_resched_softirq() will enable bhs before scheduling.
  */
+void rcu_all_qs(void);
 #ifndef CONFIG_PREEMPT
 extern int _cond_resched(void);
 #else
-static inline int _cond_resched(void) { return 0; }
+static inline int _cond_resched(void) { rcu_all_qs(); return 0; }
 #endif
 
 #define cond_resched() ({			\

kernel/rcu/tree.c +5 -20

index 51d4c3acf32d..e40cb5190783 100644
@@ -537,8 +537,8 @@ module_param(rcu_kick_kthreads, bool, 0644);
  * How long the grace period must be before we start recruiting
  * quiescent-state help from rcu_note_context_switch().
  */
-static ulong jiffies_till_sched_qs = HZ / 20;
-module_param(jiffies_till_sched_qs, ulong, 0644);
+static ulong jiffies_till_sched_qs = HZ / 10;
+module_param(jiffies_till_sched_qs, ulong, 0444);
 
 static bool rcu_start_gp_advanced(struct rcu_state *rsp, struct rcu_node *rnp,
 				  struct rcu_data *rdp);

@@ -1230,7 +1230,6 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
 	unsigned long jtsq;
 	bool *rnhqp;
 	bool *ruqp;
-	unsigned long rjtsc;
 	struct rcu_node *rnp;
 
 	/*

@@ -1247,23 +1246,13 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
 		return 1;
 	}
 
-	/* Compute and saturate jiffies_till_sched_qs. */
-	jtsq = jiffies_till_sched_qs;
-	rjtsc = rcu_jiffies_till_stall_check();
-	if (jtsq > rjtsc / 2) {
-		WRITE_ONCE(jiffies_till_sched_qs, rjtsc);
-		jtsq = rjtsc / 2;
-	} else if (jtsq < 1) {
-		WRITE_ONCE(jiffies_till_sched_qs, 1);
-		jtsq = 1;
-	}
-
 	/*
 	 * Has this CPU encountered a cond_resched_rcu_qs() since the
 	 * beginning of the grace period?  For this to be the case,
 	 * the CPU has to have noticed the current grace period.  This
 	 * might not be the case for nohz_full CPUs looping in the kernel.
 	 */
+	jtsq = jiffies_till_sched_qs;
 	rnp = rdp->mynode;
 	ruqp = per_cpu_ptr(&rcu_dynticks.rcu_urgent_qs, rdp->cpu);
 	if (time_after(jiffies, rdp->rsp->gp_start + jtsq) &&

@@ -1271,7 +1260,7 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
 	    READ_ONCE(rdp->gpnum) == rnp->gpnum && !rdp->gpwrap) {
 		trace_rcu_fqs(rdp->rsp->name, rdp->gpnum, rdp->cpu, TPS("rqc"));
 		return 1;
-	} else {
+	} else if (time_after(jiffies, rdp->rsp->gp_start + jtsq)) {
 		/* Load rcu_qs_ctr before store to rcu_urgent_qs. */
 		smp_store_release(ruqp, true);
 	}

@@ -1299,10 +1288,6 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
 	 * updates are only once every few jiffies, the probability of
 	 * lossage (and thus of slight grace-period extension) is
 	 * quite low.
-	 *
-	 * Note that if the jiffies_till_sched_qs boot/sysfs parameter
-	 * is set too high, we override with half of the RCU CPU stall
-	 * warning delay.
 	 */
 	rnhqp = &per_cpu(rcu_dynticks.rcu_need_heavy_qs, rdp->cpu);
 	if (!READ_ONCE(*rnhqp) &&

@@ -1311,7 +1296,7 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
 		WRITE_ONCE(*rnhqp, true);
 		/* Store rcu_need_heavy_qs before rcu_urgent_qs. */
 		smp_store_release(ruqp, true);
-		rdp->rsp->jiffies_resched += 5; /* Re-enable beating. */
+		rdp->rsp->jiffies_resched += jtsq; /* Re-enable beating. */
 	}
 
 	/*

kernel/sched/core.c +0 -0

index 17c667b427b4..9433633012ba 100644

Quoted 117 lines · Paul E. McKenney

There is some confusion as to which of cond_resched() or cond_resched_rcu_qs() should be added to long in-kernel loops. This commit therefore eliminates the decision by adding RCU quiescent states to cond_resched(). This commit also simplifies the code that used to interact with cond_resched_rcu_qs(), and that now interacts with cond_resched(), to reduce its overhead. This reduction is necessary to allow the heavier-weight cond_resched_rcu_qs() mechanism to be invoked everywhere that cond_resched() is invoked.

Part of that reduction in overhead converts the jiffies_till_sched_qs kernel parameter to read-only at runtime, thus eliminating the need for bounds checking.

Reported-by Michal Hocko <[email protected]>

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

Cc Peter Zijlstra <[email protected]>

[ paulmck: Tuning for performance issues reported by 0day Test Robot. ]

include/linux/sched.h +2 -1

index 8337e2db0bb2..d2f291a3a44a 100644
@@ -1522,10 +1522,11 @@ static inline int test_tsk_need_resched(struct task_struct *tsk)
  * cond_resched_lock() will drop the spinlock before scheduling,
  * cond_resched_softirq() will enable bhs before scheduling.
  */
+void rcu_all_qs(void);
 #ifndef CONFIG_PREEMPT
 extern int _cond_resched(void);
 #else
-static inline int _cond_resched(void) { return 0; }
+static inline int _cond_resched(void) { rcu_all_qs(); return 0; }
 #endif
 
 #define cond_resched() ({			\

kernel/rcu/tree.c +5 -20

index 51d4c3acf32d..e40cb5190783 100644
@@ -537,8 +537,8 @@ module_param(rcu_kick_kthreads, bool, 0644);
  * How long the grace period must be before we start recruiting
  * quiescent-state help from rcu_note_context_switch().
  */
-static ulong jiffies_till_sched_qs = HZ / 20;
-module_param(jiffies_till_sched_qs, ulong, 0644);
+static ulong jiffies_till_sched_qs = HZ / 10;
+module_param(jiffies_till_sched_qs, ulong, 0444);
 
 static bool rcu_start_gp_advanced(struct rcu_state *rsp, struct rcu_node *rnp,
 				  struct rcu_data *rdp);

@@ -1230,7 +1230,6 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
 	unsigned long jtsq;
 	bool *rnhqp;
 	bool *ruqp;
-	unsigned long rjtsc;
 	struct rcu_node *rnp;
 
 	/*

@@ -1247,23 +1246,13 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
 		return 1;
 	}
 
-	/* Compute and saturate jiffies_till_sched_qs. */
-	jtsq = jiffies_till_sched_qs;
-	rjtsc = rcu_jiffies_till_stall_check();
-	if (jtsq > rjtsc / 2) {
-		WRITE_ONCE(jiffies_till_sched_qs, rjtsc);
-		jtsq = rjtsc / 2;
-	} else if (jtsq < 1) {
-		WRITE_ONCE(jiffies_till_sched_qs, 1);
-		jtsq = 1;
-	}
-
 	/*
 	 * Has this CPU encountered a cond_resched_rcu_qs() since the
 	 * beginning of the grace period?  For this to be the case,
 	 * the CPU has to have noticed the current grace period.  This
 	 * might not be the case for nohz_full CPUs looping in the kernel.
 	 */
+	jtsq = jiffies_till_sched_qs;
 	rnp = rdp->mynode;
 	ruqp = per_cpu_ptr(&rcu_dynticks.rcu_urgent_qs, rdp->cpu);
 	if (time_after(jiffies, rdp->rsp->gp_start + jtsq) &&

@@ -1271,7 +1260,7 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
 	    READ_ONCE(rdp->gpnum) == rnp->gpnum && !rdp->gpwrap) {
 		trace_rcu_fqs(rdp->rsp->name, rdp->gpnum, rdp->cpu, TPS("rqc"));
 		return 1;
-	} else {
+	} else if (time_after(jiffies, rdp->rsp->gp_start + jtsq)) {
 		/* Load rcu_qs_ctr before store to rcu_urgent_qs. */
 		smp_store_release(ruqp, true);
 	}

@@ -1299,10 +1288,6 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
 	 * updates are only once every few jiffies, the probability of
 	 * lossage (and thus of slight grace-period extension) is
 	 * quite low.
-	 *
-	 * Note that if the jiffies_till_sched_qs boot/sysfs parameter
-	 * is set too high, we override with half of the RCU CPU stall
-	 * warning delay.
 	 */
 	rnhqp = &per_cpu(rcu_dynticks.rcu_need_heavy_qs, rdp->cpu);
 	if (!READ_ONCE(*rnhqp) &&

@@ -1311,7 +1296,7 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
 		WRITE_ONCE(*rnhqp, true);
 		/* Store rcu_need_heavy_qs before rcu_urgent_qs. */
 		smp_store_release(ruqp, true);
-		rdp->rsp->jiffies_resched += 5; /* Re-enable beating. */
+		rdp->rsp->jiffies_resched += jtsq; /* Re-enable beating. */
 	}
 
 	/*

kernel/sched/core.c +0 -0

index 17c667b427b4..9433633012ba 100644

kernel/sched/core.c int __sched _cond_resched(void) · line 4814

Quoted 7 lines

kernel/sched/core.c +1 -0

@@ -4808,6 +4808,7 @@ int __sched _cond_resched(void)
 		preempt_schedule_common();
 		return 1;
 	}
+	rcu_all_qs();
 	return 0;
 }
 EXPORT_SYMBOL(_cond_resched);

So I'm a bit uneasy about this change:

- There's hundreds of uses of cond_resched(), some of them in commonly inlined functions.

- cond_resched() typically gets called in functions that _might_ take a long time to execute, but that's not a given.

- it's definitely getting called opportunistically as well, under PREEMPT_VOLUNTARY, from common lightweight helpers that we know are in schedulable contexts. We risk adding significant overhead here.

So what we risk here is turning a known to be super simple function into something much slower - and exporting slowdowns to literally thousands of explicit and implicit usage sites.

The handling of RCU's no-CBs CPUs has a maintenance headache, namely that if call_rcu() is invoked with interrupts disabled, the rcuo kthread wakeup must be defered to a point where we can be sure that scheduler locks are not held. Of course, there are a lot of code paths leading from an interrupts-disabled invocation of call_rcu(), and missing any one of these can result in excessive callback-invocation latency, and potentially even system hangs.

This commit therefore uses a timer to guarantee that the wakeup will eventually occur. If one of the deferred-wakeup points kicks in, then the timer is simply cancelled.

This commit also fixes up an incomplete removal of commits that were intended to plug remaining exit paths, which should have the added benefit of reducing the overhead of RCU's context-switch hooks. In addition, it simplifies leader-to-follower callback-list handoff by introducing locking. The call_rcu()-to-leader handoff continues to use atomic operations in order to maintain good real-time latency for common-case use of call_rcu().

Quoted 7 lines · On Mon, 24 Jul 2017 14:44:31 -0700 "Paul E. McKenney"…

The handling of RCU's no-CBs CPUs has a maintenance headache, namely that if call_rcu() is invoked with interrupts disabled, the rcuo kthread wakeup must be defered to a point where we can be sure that scheduler locks are not held. Of course, there are a lot of code paths leading from an interrupts-disabled invocation of call_rcu(), and missing any one of these can result in excessive callback-invocation latency, and potentially even system hangs.

What about using irq_work? That's what perf and ftrace use for such a case.

-- Steve

Quoted 13 lines

This commit therefore uses a timer to guarantee that the wakeup will eventually occur. If one of the deferred-wakeup points kicks in, then the timer is simply cancelled.

This commit also fixes up an incomplete removal of commits that were intended to plug remaining exit paths, which should have the added benefit of reducing the overhead of RCU's context-switch hooks. In addition, it simplifies leader-to-follower callback-list handoff by introducing locking. The call_rcu()-to-leader handoff continues to use atomic operations in order to maintain good real-time latency for common-case use of call_rcu().

Quoted 11 lines · Steven Rostedt

Quoted 7 lines · On Mon, 24 Jul 2017 14:44:31 -0700 "Paul E. McKenney"…

The handling of RCU's no-CBs CPUs has a maintenance headache, namely that if call_rcu() is invoked with interrupts disabled, the rcuo kthread wakeup must be defered to a point where we can be sure that scheduler locks are not held. Of course, there are a lot of code paths leading from an interrupts-disabled invocation of call_rcu(), and missing any one of these can result in excessive callback-invocation latency, and potentially even system hangs.

Quoted 2 lines

What about using irq_work? That's what perf and ftrace use for such a case.

I hadn't looked at irq_work before, thank you for the pointer!

I nevertheless believe that timers work better in this particular case because they can be cancelled (which appears to be the common case), they

normally are not at all time-critical, and because running in softirq is just fine -- no need to run out of the scheduling-clock interrupt.

Seem reasonable?

Quoted 15 lines · On Tue, 25 Jul 2017 12:18:14 -0700 "Paul E. McKenney"…

Quoted 13 lines · Steven Rostedt

Quoted 7 lines · On Mon, 24 Jul 2017 14:44:31 -0700 "Paul E. McKenney"…

The handling of RCU's no-CBs CPUs has a maintenance headache, namely that if call_rcu() is invoked with interrupts disabled, the rcuo kthread wakeup must be defered to a point where we can be sure that scheduler locks are not held. Of course, there are a lot of code paths leading from an interrupts-disabled invocation of call_rcu(), and missing any one of these can result in excessive callback-invocation latency, and potentially even system hangs.

What about using irq_work? That's what perf and ftrace use for such a case.

Quoted 4 lines

I hadn't looked at irq_work before, thank you for the pointer!

I nevertheless believe that timers work better in this particular case because they can be cancelled (which appears to be the common case), they

Is the common case here that it doesn't trigger? That is, the del_timer() will be called?

Quoted 2 lines

normally are not at all time-critical, and because running in softirq is just fine -- no need to run out of the scheduling-clock interrupt.

irq_work doesn't always use the scheduling clock. IIRC, it will simply trigger a interrupt (if the arch supports it), and the work will be done when interrupts are enabled (the interrupt that will do the work will trigger)

Quoted 1 line

Seem reasonable?

Don't know. With irq_work, you just call it and forget about it. No need to mod or del timers.

Quoted 23 lines · Steven Rostedt

Quoted 19 lines · On Tue, 25 Jul 2017 12:18:14 -0700 "Paul E. McKenney"…

Quoted 11 lines · Steven Rostedt

Quoted 7 lines · On Mon, 24 Jul 2017 14:44:31 -0700 "Paul E. McKenney"…

The handling of RCU's no-CBs CPUs has a maintenance headache, namely that if call_rcu() is invoked with interrupts disabled, the rcuo kthread wakeup must be defered to a point where we can be sure that scheduler locks are not held. Of course, there are a lot of code paths leading from an interrupts-disabled invocation of call_rcu(), and missing any one of these can result in excessive callback-invocation latency, and potentially even system hangs.

Quoted 2 lines

What about using irq_work? That's what perf and ftrace use for such a case.

I hadn't looked at irq_work before, thank you for the pointer!

I nevertheless believe that timers work better in this particular case because they can be cancelled (which appears to be the common case), they

Quoted 2 lines

Is the common case here that it doesn't trigger? That is, the del_timer() will be called?

If you have lots of call_rcu() invocations, many of them will be invoked with interrupts enabled, and a later one with interrupts enabled will take care of things for the earlier ones. So there can be workloads where this is the case.

Quoted 8 lines

Quoted 3 lines

normally are not at all time-critical, and because running in softirq is just fine -- no need to run out of the scheduling-clock interrupt.

irq_work doesn't always use the scheduling clock. IIRC, it will simply trigger a interrupt (if the arch supports it), and the work will be done when interrupts are enabled (the interrupt that will do the work will trigger)

Ah, OK, so scheduling clock is just the backstop. Still, softirq is a bit nicer to manage than hardirq.

Quoted 5 lines

Quoted 2 lines

Seem reasonable?

Don't know. With irq_work, you just call it and forget about it. No need to mod or del timers.

But I could have a series of call_rcu() invocations with interrupts disabled, so I would need to interact somehow with the irq_work handler. Either that or dynamically allocate the needed data structure.

Or am I missing something here?

Quoted 27 lines · On Tue, 25 Jul 2017 17:05:40 -0700 "Paul E. McKenney"…

Quoted 25 lines · Steven Rostedt

Quoted 12 lines · On Tue, 25 Jul 2017 12:18:14 -0700 "Paul E. McKenney"…

Quoted 11 lines · Steven Rostedt

Quoted 7 lines · On Mon, 24 Jul 2017 14:44:31 -0700 "Paul E. McKenney"…

The handling of RCU's no-CBs CPUs has a maintenance headache, namely that if call_rcu() is invoked with interrupts disabled, the rcuo kthread wakeup must be defered to a point where we can be sure that scheduler locks are not held. Of course, there are a lot of code paths leading from an interrupts-disabled invocation of call_rcu(), and missing any one of these can result in excessive callback-invocation latency, and potentially even system hangs.

Quoted 7 lines

Quoted 2 lines

What about using irq_work? That's what perf and ftrace use for such a case.

I hadn't looked at irq_work before, thank you for the pointer!

I nevertheless believe that timers work better in this particular case because they can be cancelled (which appears to be the common case), they

Is the common case here that it doesn't trigger? That is, the del_timer() will be called?

Quoted 4 lines

If you have lots of call_rcu() invocations, many of them will be invoked with interrupts enabled, and a later one with interrupts enabled will take care of things for the earlier ones. So there can be workloads where this is the case.

Note, only the first irq_work called will take action. The other callers will see that a irq_work is pending and will not reivoke one.

Quoted 8 lines

Quoted 7 lines

Quoted 2 lines

normally are not at all time-critical, and because running in softirq is just fine -- no need to run out of the scheduling-clock interrupt.

irq_work doesn't always use the scheduling clock. IIRC, it will simply trigger a interrupt (if the arch supports it), and the work will be done when interrupts are enabled (the interrupt that will do the work will trigger)

Quoted 2 lines

Ah, OK, so scheduling clock is just the backstop. Still, softirq is a bit nicer to manage than hardirq.

Still requires a hard interrupt (timer) (thinking of NOHZ FULL where this does matter).

Quoted 10 lines

Quoted 4 lines

Quoted 1 line

Seem reasonable?

Don't know. With irq_work, you just call it and forget about it. No need to mod or del timers.

But I could have a series of call_rcu() invocations with interrupts disabled, so I would need to interact somehow with the irq_work handler. Either that or dynamically allocate the needed data structure.

Or am I missing something here?

You treat it just like you are with the timer code. You have a irq_work struct attached to your rdp descriptor. And call irq_work_run() when interrupts are disabled. If it hasn't already been invoked it will invoke one. Then the irq_work handler will look at the rdp attached to the irq_work (container_of()), and then wake the associated thread.

It is much lighter weight than a timer setup.

Quoted 1 line

It is much lighter weight than a timer setup.

How much lighter weight? In other words, what fraction of the timers have to avoid being cancelled for irq_work to break even?

Quoted 4 lines · On Wed, 26 Jul 2017 14:47:41 -0700 "Paul E. McKenney"…

Quoted 1 line

It is much lighter weight than a timer setup.

How much lighter weight? In other words, what fraction of the timers have to avoid being cancelled for irq_work to break even?

No idea. I guess that would be a nice academic exercise ;-)

The actual use of TASKS_RCU is only when PREEMPT, otherwise RCU-sched is used instead. This commit therefore makes synchronize_rcu_tasks() and call_rcu_tasks() available always, but mapped to synchronize_sched() and call_rcu_sched(), respectively, when !PREEMPT. This approach also allows some #ifdefs to be removed from rcutorture.

Quoted 5 lines · On Mon, 24 Jul 2017 14:44:32 -0700 "Paul E. McKenney"…

The actual use of TASKS_RCU is only when PREEMPT, otherwise RCU-sched is used instead. This commit therefore makes synchronize_rcu_tasks() and call_rcu_tasks() available always, but mapped to synchronize_sched() and call_rcu_sched(), respectively, when !PREEMPT. This approach also allows some #ifdefs to be removed from rcutorture.

Hmm, I'll need to update ftrace. I believe I call synchronize_sched() twice with !PREEMPT then.

Strings used in event tracing need to be specially handled, for example, using the TPS() macro. Without the TPS() macro, although output looks fine from within a running kernel, extracting traces from a crash dump produces garbage instead of strings. This commit therefore adds the TPS() macro to some unadorned strings that were passed to event-tracing macros.

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

Cc Steven Rostedt <[email protected]>

---
 kernel/rcu/tree_plugin.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

kernel/rcu/tree_plugin.h +6 -6

index bb9e6e43130f..14ba496a13cd 100644
@@ -2073,7 +2073,7 @@ static void nocb_leader_wait(struct rcu_data *my_rdp)
 
 	/* Wait for callbacks to appear. */
 	if (!rcu_nocb_poll) {
-		trace_rcu_nocb_wake(my_rdp->rsp->name, my_rdp->cpu, "Sleep");
+		trace_rcu_nocb_wake(my_rdp->rsp->name, my_rdp->cpu, TPS("Sleep"));
 		swait_event_interruptible(my_rdp->nocb_wq,
 				!READ_ONCE(my_rdp->nocb_leader_sleep));
 		raw_spin_lock_irqsave(&my_rdp->nocb_lock, flags);

@@ -2083,7 +2083,7 @@ static void nocb_leader_wait(struct rcu_data *my_rdp)
 		raw_spin_unlock_irqrestore(&my_rdp->nocb_lock, flags);
 	} else if (firsttime) {
 		firsttime = false; /* Don't drown trace log with "Poll"! */
-		trace_rcu_nocb_wake(my_rdp->rsp->name, my_rdp->cpu, "Poll");
+		trace_rcu_nocb_wake(my_rdp->rsp->name, my_rdp->cpu, TPS("Poll"));
 	}
 
 	/*

@@ -2111,7 +2111,7 @@ static void nocb_leader_wait(struct rcu_data *my_rdp)
 			schedule_timeout_interruptible(1);
 		} else {
 			trace_rcu_nocb_wake(my_rdp->rsp->name, my_rdp->cpu,
-					    "WokeEmpty");
+					    TPS("WokeEmpty"));
 		}
 		goto wait_again;
 	}

@@ -2155,7 +2155,7 @@ static void nocb_leader_wait(struct rcu_data *my_rdp)
 static void nocb_follower_wait(struct rcu_data *rdp)
 {
 	for (;;) {
-		trace_rcu_nocb_wake(rdp->rsp->name, rdp->cpu, "FollowerSleep");
+		trace_rcu_nocb_wake(rdp->rsp->name, rdp->cpu, TPS("FollowerSleep"));
 		swait_event_interruptible(rdp->nocb_wq,
 					 READ_ONCE(rdp->nocb_follower_head));
 		if (smp_load_acquire(&rdp->nocb_follower_head)) {

@@ -2163,7 +2163,7 @@ static void nocb_follower_wait(struct rcu_data *rdp)
 			return;
 		}
 		WARN_ON(signal_pending(current));
-		trace_rcu_nocb_wake(rdp->rsp->name, rdp->cpu, "WokeEmpty");
+		trace_rcu_nocb_wake(rdp->rsp->name, rdp->cpu, TPS("WokeEmpty"));
 	}
 }
 

@@ -2198,7 +2198,7 @@ static int rcu_nocb_kthread(void *arg)
 		rdp->nocb_follower_tail = &rdp->nocb_follower_head;
 		raw_spin_unlock_irqrestore(&rdp->nocb_lock, flags);
 		BUG_ON(!list);
-		trace_rcu_nocb_wake(rdp->rsp->name, rdp->cpu, "WokeNonEmpty");
+		trace_rcu_nocb_wake(rdp->rsp->name, rdp->cpu, TPS("WokeNonEmpty"));
 
 		/* Each pass through the following loop invokes a callback. */
 		trace_rcu_batch_start(rdp->rsp->name,

There is currently event tracing to track when a task is preempted within a preemptible RCU read-side critical section, and also when that task subsequently reaches its outermost rcu_read_unlock(), but none indicating when a new grace period starts when that grace period must wait on pre-existing readers that have been been preempted at least once since the beginning of their current RCU read-side critical sections.

This commit therefore adds an event trace at grace-period start in the case where there are such readers. Note that only the first reader in the list is traced.

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

---
 kernel/rcu/tree_plugin.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

kernel/rcu/tree_plugin.h +0 -0

index 14ba496a13cd..3e3f92e981a1 100644

kernel/rcu/tree_plugin.h +5 -0

 		rnp->gp_tasks = rnp->blkd_tasks.next;
+		t = container_of(rnp->gp_tasks, struct task_struct,
+				 rcu_node_entry);
+		trace_rcu_unlock_preempted_task(TPS("rcu_preempt-GPS"),
+						rnp->gpnum, t->pid);
+	}
 	WARN_ON_ONCE(rnp->qsmask);
 }
 

Quoted 21 lines · On Mon, 24 Jul 2017 14:44:36 -0700 "Paul E. McKenney"…

There is currently event tracing to track when a task is preempted within a preemptible RCU read-side critical section, and also when that task subsequently reaches its outermost rcu_read_unlock(), but none indicating when a new grace period starts when that grace period must wait on pre-existing readers that have been been preempted at least once since the beginning of their current RCU read-side critical sections.

This commit therefore adds an event trace at grace-period start in the case where there are such readers. Note that only the first reader in the list is traced.

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

---
 kernel/rcu/tree_plugin.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

kernel/rcu/tree_plugin.h +0 -0

index 14ba496a13cd..3e3f92e981a1 100644

kernel/rcu/tree_plugin.h static int rcu_print_task_exp_stall(struct rcu_node *rnp) · line 643

Quoted 9 lines

kernel/rcu/tree_plugin.h +3 -1

@@ -636,10 +636,17 @@ static int rcu_print_task_exp_stall(struct rcu_node *rnp)
  */
 static void rcu_preempt_check_blocked_tasks(struct rcu_node *rnp)
 {
+	struct task_struct *t;
+
 	RCU_LOCKDEP_WARN(preemptible(), "rcu_preempt_check_blocked_tasks() invoked with preemption enabled!!!\n");
 	WARN_ON_ONCE(rcu_preempt_blocked_readers_cgp(rnp));
-	if (rcu_preempt_has_tasks(rnp))
+	if (rcu_preempt_has_tasks(rnp)) {

The only function of this if block is to fill the content of the trace event, correct?

What about doing:

	if (trace_rcu_unlock_preempted_task_enabled() &&
	    rcu_preempt_has_tasks(rnp)) {

instead? The trace_rcu_unlock_preempted_task_enabled() is a static branch (aka jump_label), which would make the above a constant branch when tracing is not enabled, and would keep this from adding any extra overhead.

-- Steve

Quoted 1 line

kernel/rcu/tree_plugin.h +0 -0

 		rnp->gp_tasks = rnp->blkd_tasks.next;

Quoted 8 lines

kernel/rcu/tree_plugin.h +5 -0

+		t = container_of(rnp->gp_tasks, struct task_struct,
+				 rcu_node_entry);
+		trace_rcu_unlock_preempted_task(TPS("rcu_preempt-GPS"),
+						rnp->gpnum, t->pid);
+	}
 	WARN_ON_ONCE(rnp->qsmask);
 }
 

Quoted 42 lines · Steven Rostedt

Quoted 21 lines · On Mon, 24 Jul 2017 14:44:36 -0700 "Paul E. McKenney"…

There is currently event tracing to track when a task is preempted within a preemptible RCU read-side critical section, and also when that task subsequently reaches its outermost rcu_read_unlock(), but none indicating when a new grace period starts when that grace period must wait on pre-existing readers that have been been preempted at least once since the beginning of their current RCU read-side critical sections.

This commit therefore adds an event trace at grace-period start in the case where there are such readers. Note that only the first reader in the list is traced.

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

---
 kernel/rcu/tree_plugin.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

kernel/rcu/tree_plugin.h +0 -0

index 14ba496a13cd..3e3f92e981a1 100644

kernel/rcu/tree_plugin.h static int rcu_print_task_exp_stall(struct rcu_node *rnp) · line 643

Quoted 9 lines

kernel/rcu/tree_plugin.h +3 -1

@@ -636,10 +636,17 @@ static int rcu_print_task_exp_stall(struct rcu_node *rnp)
  */
 static void rcu_preempt_check_blocked_tasks(struct rcu_node *rnp)
 {
+	struct task_struct *t;
+
 	RCU_LOCKDEP_WARN(preemptible(), "rcu_preempt_check_blocked_tasks() invoked with preemption enabled!!!\n");
 	WARN_ON_ONCE(rcu_preempt_blocked_readers_cgp(rnp));
-	if (rcu_preempt_has_tasks(rnp))
+	if (rcu_preempt_has_tasks(rnp)) {  

The only function of this if block is to fill the content of the trace event, correct?

What about doing:

	if (trace_rcu_unlock_preempted_task_enabled() &&
	    rcu_preempt_has_tasks(rnp)) {

Quoted 8 lines

instead? The trace_rcu_unlock_preempted_task_enabled() is a static branch (aka jump_label), which would make the above a constant branch when tracing is not enabled, and would keep this from adding any extra overhead.

-- Steve

Quoted 1 line

kernel/rcu/tree_plugin.h +0 -0

 		rnp->gp_tasks = rnp->blkd_tasks.next;  

The trace_rcu_unlock_preempted_task_enabled() call is a new one on me, thank you!

Unfortunately, the above assignment to rnp->gp_tasks is required even if tracing is disabled. The reason is that the newly started grace period needs to wait on all tasks that have been preempted within their current RCU read-side critical section, and rnp->gp_tasks records the point in the rnp->blkd_tasks list beyond which all preempted tasks block this new grace period.

If this assignment is omitted, we get too-short grace periods, and the tasks on this list might still be holding references to stuff that gets freed at the end of this new grace period.

I applied your two acks, thank you!

Quoted 66 lines · On Thu, 27 Jul 2017 20:22:32 -0700 "Paul E. McKenney"…

Quoted 42 lines · Steven Rostedt

Quoted 21 lines · On Mon, 24 Jul 2017 14:44:36 -0700 "Paul E. McKenney"…

There is currently event tracing to track when a task is preempted within a preemptible RCU read-side critical section, and also when that task subsequently reaches its outermost rcu_read_unlock(), but none indicating when a new grace period starts when that grace period must wait on pre-existing readers that have been been preempted at least once since the beginning of their current RCU read-side critical sections.

This commit therefore adds an event trace at grace-period start in the case where there are such readers. Note that only the first reader in the list is traced.

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

---
 kernel/rcu/tree_plugin.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

kernel/rcu/tree_plugin.h +0 -0

index 14ba496a13cd..3e3f92e981a1 100644

kernel/rcu/tree_plugin.h static int rcu_print_task_exp_stall(struct rcu_node *rnp) · line 643

Quoted 9 lines

kernel/rcu/tree_plugin.h +3 -1

@@ -636,10 +636,17 @@ static int rcu_print_task_exp_stall(struct rcu_node *rnp)
  */
 static void rcu_preempt_check_blocked_tasks(struct rcu_node *rnp)
 {
+	struct task_struct *t;
+
 	RCU_LOCKDEP_WARN(preemptible(), "rcu_preempt_check_blocked_tasks() invoked with preemption enabled!!!\n");
 	WARN_ON_ONCE(rcu_preempt_blocked_readers_cgp(rnp));
-	if (rcu_preempt_has_tasks(rnp))
+	if (rcu_preempt_has_tasks(rnp)) {  

The only function of this if block is to fill the content of the trace event, correct?

What about doing:

	if (trace_rcu_unlock_preempted_task_enabled() &&
	    rcu_preempt_has_tasks(rnp)) {

Quoted 8 lines

instead? The trace_rcu_unlock_preempted_task_enabled() is a static branch (aka jump_label), which would make the above a constant branch when tracing is not enabled, and would keep this from adding any extra overhead.

-- Steve

Quoted 1 line

kernel/rcu/tree_plugin.h +0 -0

 		rnp->gp_tasks = rnp->blkd_tasks.next;  

The trace_rcu_unlock_preempted_task_enabled() call is a new one on me, thank you!

Unfortunately, the above assignment to rnp->gp_tasks is required even if tracing is disabled. The reason is that the newly started grace period needs to wait on all tasks that have been preempted within their current RCU read-side critical section, and rnp->gp_tasks records the point in the rnp->blkd_tasks list beyond which all preempted tasks block this new grace period.

If this assignment is omitted, we get too-short grace periods, and the tasks on this list might still be holding references to stuff that gets freed at the end of this new grace period.

Quoted 2 lines

I applied your two acks, thank you!

And with you answer about the block not just being for tracing, you can add my acked-by here too ;-)

The _rcu_barrier_trace() function is a wrapper for trace_rcu_barrier(), which needs TPS() protection for strings passed through the second argument. However, it has escaped prior TPS()-ification efforts because it _rcu_barrier_trace() does not start with "trace_". This commit therefore adds the needed TPS() protection

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

Cc Steven Rostedt <[email protected]>

---
 kernel/rcu/tree.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

kernel/rcu/tree.c +15 -12

index 9a90d9b1dc04..7e018696fd82 100644
@@ -3553,10 +3553,11 @@ static void rcu_barrier_callback(struct rcu_head *rhp)
 	struct rcu_state *rsp = rdp->rsp;
 
 	if (atomic_dec_and_test(&rsp->barrier_cpu_count)) {
-		_rcu_barrier_trace(rsp, "LastCB", -1, rsp->barrier_sequence);
+		_rcu_barrier_trace(rsp, TPS("LastCB"), -1,
+				   rsp->barrier_sequence);
 		complete(&rsp->barrier_completion);
 	} else {
-		_rcu_barrier_trace(rsp, "CB", -1, rsp->barrier_sequence);
+		_rcu_barrier_trace(rsp, TPS("CB"), -1, rsp->barrier_sequence);
 	}
 }
 

@@ -3568,14 +3569,15 @@ static void rcu_barrier_func(void *type)
 	struct rcu_state *rsp = type;
 	struct rcu_data *rdp = raw_cpu_ptr(rsp->rda);
 
-	_rcu_barrier_trace(rsp, "IRQ", -1, rsp->barrier_sequence);
+	_rcu_barrier_trace(rsp, TPS("IRQ"), -1, rsp->barrier_sequence);
 	rdp->barrier_head.func = rcu_barrier_callback;
 	debug_rcu_head_queue(&rdp->barrier_head);
 	if (rcu_segcblist_entrain(&rdp->cblist, &rdp->barrier_head, 0)) {
 		atomic_inc(&rsp->barrier_cpu_count);
 	} else {
 		debug_rcu_head_unqueue(&rdp->barrier_head);
-		_rcu_barrier_trace(rsp, "IRQNQ", -1, rsp->barrier_sequence);
+		_rcu_barrier_trace(rsp, TPS("IRQNQ"), -1,
+				   rsp->barrier_sequence);
 	}
 }
 

@@ -3589,14 +3591,15 @@ static void _rcu_barrier(struct rcu_state *rsp)
 	struct rcu_data *rdp;
 	unsigned long s = rcu_seq_snap(&rsp->barrier_sequence);
 
-	_rcu_barrier_trace(rsp, "Begin", -1, s);
+	_rcu_barrier_trace(rsp, TPS("Begin"), -1, s);
 
 	/* Take mutex to serialize concurrent rcu_barrier() requests. */
 	mutex_lock(&rsp->barrier_mutex);
 
 	/* Did someone else do our work for us? */
 	if (rcu_seq_done(&rsp->barrier_sequence, s)) {
-		_rcu_barrier_trace(rsp, "EarlyExit", -1, rsp->barrier_sequence);
+		_rcu_barrier_trace(rsp, TPS("EarlyExit"), -1,
+				   rsp->barrier_sequence);
 		smp_mb(); /* caller's subsequent code after above check. */
 		mutex_unlock(&rsp->barrier_mutex);
 		return;

@@ -3604,7 +3607,7 @@ static void _rcu_barrier(struct rcu_state *rsp)
 
 	/* Mark the start of the barrier operation. */
 	rcu_seq_start(&rsp->barrier_sequence);
-	_rcu_barrier_trace(rsp, "Inc1", -1, rsp->barrier_sequence);
+	_rcu_barrier_trace(rsp, TPS("Inc1"), -1, rsp->barrier_sequence);
 
 	/*
 	 * Initialize the count to one rather than to zero in order to

@@ -3627,10 +3630,10 @@ static void _rcu_barrier(struct rcu_state *rsp)
 		rdp = per_cpu_ptr(rsp->rda, cpu);
 		if (rcu_is_nocb_cpu(cpu)) {
 			if (!rcu_nocb_cpu_needs_barrier(rsp, cpu)) {
-				_rcu_barrier_trace(rsp, "OfflineNoCB", cpu,
+				_rcu_barrier_trace(rsp, TPS("OfflineNoCB"), cpu,
 						   rsp->barrier_sequence);
 			} else {
-				_rcu_barrier_trace(rsp, "OnlineNoCB", cpu,
+				_rcu_barrier_trace(rsp, TPS("OnlineNoCB"), cpu,
 						   rsp->barrier_sequence);
 				smp_mb__before_atomic();
 				atomic_inc(&rsp->barrier_cpu_count);

@@ -3638,11 +3641,11 @@ static void _rcu_barrier(struct rcu_state *rsp)
 					   rcu_barrier_callback, rsp, cpu, 0);
 			}
 		} else if (rcu_segcblist_n_cbs(&rdp->cblist)) {
-			_rcu_barrier_trace(rsp, "OnlineQ", cpu,
+			_rcu_barrier_trace(rsp, TPS("OnlineQ"), cpu,
 					   rsp->barrier_sequence);
 			smp_call_function_single(cpu, rcu_barrier_func, rsp, 1);
 		} else {
-			_rcu_barrier_trace(rsp, "OnlineNQ", cpu,
+			_rcu_barrier_trace(rsp, TPS("OnlineNQ"), cpu,
 					   rsp->barrier_sequence);
 		}
 	}

@@ -3659,7 +3662,7 @@ static void _rcu_barrier(struct rcu_state *rsp)
 	wait_for_completion(&rsp->barrier_completion);
 
 	/* Mark the end of the barrier operation. */
-	_rcu_barrier_trace(rsp, "Inc2", -1, rsp->barrier_sequence);
+	_rcu_barrier_trace(rsp, TPS("Inc2"), -1, rsp->barrier_sequence);
 	rcu_seq_end(&rsp->barrier_sequence);
 
 	/* Other rcu_barrier() invocations can now safely proceed. */