Miscellaneous fixes for 4.10

Although rcutorture will occasionally do a 50-millisecond grace-period delay, these delays are quite rare. And rightly so, because otherwise the read rate would be quite low. Thie means that it can be important to identify whether or not a given run contained a long-delay read. This commit therefore inserts a trace_rcu_torture_read() event to flag runs containing long delays.

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

 include/trace/events/rcu.h |  5 ++++-
 kernel/rcu/rcutorture.c    | 11 ++++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

include/trace/events/rcu.h +0 -0

index d3e756539d44..b31e05bc8e26 100644

include/trace/events/rcu.h +0 -0

 TRACE_EVENT(rcu_torture_read,
 

kernel/rcu/rcutorture.c +10 -1

index bf08fee53dc7..87c51225ceec 100644
@@ -289,15 +289,24 @@ static int rcu_torture_read_lock(void) __acquires(RCU)
 
 static void rcu_read_delay(struct torture_random_state *rrsp)
 {
+	unsigned long started;
+	unsigned long completed;
 	const unsigned long shortdelay_us = 200;
 	const unsigned long longdelay_ms = 50;
+	unsigned long long ts;
 
 	/* We want a short delay sometimes to make a reader delay the grace
 	 * period, and we want a long delay occasionally to trigger
 	 * force_quiescent_state. */
 
-	if (!(torture_random(rrsp) % (nrealreaders * 2000 * longdelay_ms)))
+	if (!(torture_random(rrsp) % (nrealreaders * 2000 * longdelay_ms))) {
+		started = cur_ops->completed();
+		ts = rcu_trace_clock_local();
 		mdelay(longdelay_ms);
+		completed = cur_ops->completed();
+		do_trace_rcu_torture_read(cur_ops->name, NULL, ts,
+					  started, completed);
+	}
 	if (!(torture_random(rrsp) % (nrealreaders * 2 * shortdelay_us)))
 		udelay(shortdelay_us);
 #ifdef CONFIG_PREEMPT

Signature · 1 line

2.5.2

Quoted 8 lines · Paul E. McKenney

Although rcutorture will occasionally do a 50-millisecond grace-period delay, these delays are quite rare. And rightly so, because otherwise the read rate would be quite low. Thie means that it can be important to identify whether or not a given run contained a long-delay read. This commit therefore inserts a trace_rcu_torture_read() event to flag runs containing long delays.

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

A couple of apparent typos below. With those fixed:

Reviewed-by Josh Triplett <[email protected]>

Quoted 4 lines

 include/trace/events/rcu.h |  5 ++++-
 kernel/rcu/rcutorture.c    | 11 ++++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

include/trace/events/rcu.h TRACE_EVENT(rcu_batch_end, · line 704

Quoted 13 lines

include/trace/events/rcu.h +4 -1

index d3e756539d44..b31e05bc8e26 100644
@@ -698,7 +698,10 @@ TRACE_EVENT(rcu_batch_end,
 /*
  * Tracepoint for rcutorture readers.  The first argument is the name
  * of the RCU flavor from rcutorture's viewpoint and the second argument
- * is the callback address.
+ * is the callback address.  The third callback is the start time in
+ * seconds, and the last two arguments are the grace period numbers
+ * and the beginning and end of the read, respectively.  Note that the
+ * callback address can be NULL.

s/third callback/third argument/?

Also, s/and the beginning/of the beginning/?

Quoted 35 lines

include/trace/events/rcu.h +0 -0

 TRACE_EVENT(rcu_torture_read,
 

kernel/rcu/rcutorture.c +10 -1

index bf08fee53dc7..87c51225ceec 100644
@@ -289,15 +289,24 @@ static int rcu_torture_read_lock(void) __acquires(RCU)
 
 static void rcu_read_delay(struct torture_random_state *rrsp)
 {
+	unsigned long started;
+	unsigned long completed;
 	const unsigned long shortdelay_us = 200;
 	const unsigned long longdelay_ms = 50;
+	unsigned long long ts;
 
 	/* We want a short delay sometimes to make a reader delay the grace
 	 * period, and we want a long delay occasionally to trigger
 	 * force_quiescent_state. */
 
-	if (!(torture_random(rrsp) % (nrealreaders * 2000 * longdelay_ms)))
+	if (!(torture_random(rrsp) % (nrealreaders * 2000 * longdelay_ms))) {
+		started = cur_ops->completed();
+		ts = rcu_trace_clock_local();
 		mdelay(longdelay_ms);
+		completed = cur_ops->completed();
+		do_trace_rcu_torture_read(cur_ops->name, NULL, ts,
+					  started, completed);
+	}
 	if (!(torture_random(rrsp) % (nrealreaders * 2 * shortdelay_us)))
 		udelay(shortdelay_us);
 #ifdef CONFIG_PREEMPT

Signature · 1 line

2.5.2

Quoted 17 lines · Josh Triplett

Quoted 8 lines · Paul E. McKenney

Although rcutorture will occasionally do a 50-millisecond grace-period delay, these delays are quite rare. And rightly so, because otherwise the read rate would be quite low. Thie means that it can be important to identify whether or not a given run contained a long-delay read. This commit therefore inserts a trace_rcu_torture_read() event to flag runs containing long delays.

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

A couple of apparent typos below. With those fixed:

Reviewed-by Josh Triplett <[email protected]>

Quoted 4 lines

 include/trace/events/rcu.h |  5 ++++-
 kernel/rcu/rcutorture.c    | 11 ++++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

Quoted 15 lines

include/trace/events/rcu.h TRACE_EVENT(rcu_batch_end, · line 704

Quoted 13 lines

include/trace/events/rcu.h +4 -1

index d3e756539d44..b31e05bc8e26 100644
@@ -698,7 +698,10 @@ TRACE_EVENT(rcu_batch_end,
 /*
  * Tracepoint for rcutorture readers.  The first argument is the name
  * of the RCU flavor from rcutorture's viewpoint and the second argument
- * is the callback address.
+ * is the callback address.  The third callback is the start time in
+ * seconds, and the last two arguments are the grace period numbers
+ * and the beginning and end of the read, respectively.  Note that the
+ * callback address can be NULL.

s/third callback/third argument/?

Good catch, fixed!

Quoted 1 line

Also, s/and the beginning/of the beginning/?

Let's see... "the last two arguments are the grace period numbers and the beginning and end of the read, respectively." -ENONSENSE for sure.

I believe that the "and" needs to become "at" as follows:

"the last two arguments are the grace period numbers at the beginning and end of the read, respectively."

Does that help?

Expedited grace periods check dyntick-idle state, and avoid sending IPIs to idle CPUs, including those running guest OSes, and, on NOHZ_FULL kernels, nohz_full CPUs. However, the kernel has been observed checking a CPU while it was non-idle, but sending the IPI after it has gone idle. This commit therefore rechecks idle state immediately before sending the IPI, refraining from IPIing CPUs that have since gone idle.

Reported-by Rik van Riel <[email protected]>

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

 kernel/rcu/tree.h     |  1 +
 kernel/rcu/tree_exp.h | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

kernel/rcu/tree.h +1 -0

index e99a5234d9ed..fe98dd24adf8 100644
@@ -404,6 +404,7 @@ struct rcu_data {
 	atomic_long_t exp_workdone1;	/* # done by others #1. */
 	atomic_long_t exp_workdone2;	/* # done by others #2. */
 	atomic_long_t exp_workdone3;	/* # done by others #3. */
+	int exp_dynticks_snap;		/* Double-check need for IPI. */
 
 	/* 7) Callback offloading. */
 #ifdef CONFIG_RCU_NOCB_CPU

kernel/rcu/tree_exp.h +11 -1

index 24343eb87b58..d3053e99fdb6 100644
@@ -358,8 +358,10 @@ static void sync_rcu_exp_select_cpus(struct rcu_state *rsp,
 			struct rcu_data *rdp = per_cpu_ptr(rsp->rda, cpu);
 			struct rcu_dynticks *rdtp = &per_cpu(rcu_dynticks, cpu);
 
+			rdp->exp_dynticks_snap =
+				atomic_add_return(0, &rdtp->dynticks);
 			if (raw_smp_processor_id() == cpu ||
-			    !(atomic_add_return(0, &rdtp->dynticks) & 0x1) ||
+			    !(rdp->exp_dynticks_snap & 0x1) ||
 			    !(rnp->qsmaskinitnext & rdp->grpmask))
 				mask_ofl_test |= rdp->grpmask;
 		}

@@ -377,9 +379,17 @@ static void sync_rcu_exp_select_cpus(struct rcu_state *rsp,
 		/* IPI the remaining CPUs for expedited quiescent state. */
 		for_each_leaf_node_possible_cpu(rnp, cpu) {
 			unsigned long mask = leaf_node_cpu_bit(rnp, cpu);
+			struct rcu_data *rdp = per_cpu_ptr(rsp->rda, cpu);
+			struct rcu_dynticks *rdtp = &per_cpu(rcu_dynticks, cpu);
+
 			if (!(mask_ofl_ipi & mask))
 				continue;
 retry_ipi:
+			if (atomic_add_return(0, &rdtp->dynticks) !=
+			    rdp->exp_dynticks_snap) {
+				mask_ofl_test |= mask;
+				continue;
+			}
 			ret = smp_call_function_single(cpu, func, rsp, 0);
 			if (!ret) {
 				mask_ofl_ipi &= ~mask;

Signature · 1 line

2.5.2

Quoted 7 lines · Paul E. McKenney

Expedited grace periods check dyntick-idle state, and avoid sending IPIs to idle CPUs, including those running guest OSes, and, on NOHZ_FULL kernels, nohz_full CPUs. However, the kernel has been observed checking a CPU while it was non-idle, but sending the IPI after it has gone idle. This commit therefore rechecks idle state immediately before sending the IPI, refraining from IPIing CPUs that have since gone idle.

Quoted 2 lines

Reported-by Rik van Riel <[email protected]>

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

atomic_add_return(0, ...) seems odd. Do you actually want that, rather than atomic_read(...)? If so, can you please document exactly why?

Quoted 8 lines · Josh Triplett

Quoted 7 lines · Paul E. McKenney

Expedited grace periods check dyntick-idle state, and avoid sending IPIs to idle CPUs, including those running guest OSes, and, on NOHZ_FULL kernels, nohz_full CPUs. However, the kernel has been observed checking a CPU while it was non-idle, but sending the IPI after it has gone idle. This commit therefore rechecks idle state immediately before sending the IPI, refraining from IPIing CPUs that have since gone idle.

Quoted 5 lines

Quoted 2 lines

Reported-by Rik van Riel <[email protected]>

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

atomic_add_return(0, ...) seems odd. Do you actually want that, rather than atomic_read(...)? If so, can you please document exactly why?

Yes that is weird. The only effective difference is that it would do a load-exclusive instead of a regular load.

Quoted 9 lines · Peter Zijlstra

Quoted 8 lines · Josh Triplett

Quoted 7 lines · Paul E. McKenney

Expedited grace periods check dyntick-idle state, and avoid sending IPIs to idle CPUs, including those running guest OSes, and, on NOHZ_FULL kernels, nohz_full CPUs. However, the kernel has been observed checking a CPU while it was non-idle, but sending the IPI after it has gone idle. This commit therefore rechecks idle state immediately before sending the IPI, refraining from IPIing CPUs that have since gone idle.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Reported-by Rik van Riel <[email protected]>

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

atomic_add_return(0, ...) seems odd. Do you actually want that, rather than atomic_read(...)? If so, can you please document exactly why?

Yes that is weird. The only effective difference is that it would do a load-exclusive instead of a regular load.

It is weird, and checking to see if it is safe to convert it and its
friends to something with less overhead is on my list. This starts
with a patch series I will post soon that consolidates all these atomic_add_return() calls into a single function, which will ease testing and other verification.

All that aside, please keep in mind that much is required from this load. It is part of a network of ordered operations that guarantee that any operation from any CPU preceding a given grace period is seen to precede any other operation from any CPU following that same grace period. And each and every CPU must agree on the order of those two operations, otherwise, RCU is broken.

In addition, please note also that these operations are nowhere near any fastpaths.

Quoted 25 lines · Paul E. McKenney

Quoted 9 lines · Peter Zijlstra

Quoted 8 lines · Josh Triplett

Quoted 7 lines · Paul E. McKenney

Expedited grace periods check dyntick-idle state, and avoid sending IPIs to idle CPUs, including those running guest OSes, and, on NOHZ_FULL kernels, nohz_full CPUs. However, the kernel has been observed checking a CPU while it was non-idle, but sending the IPI after it has gone idle. This commit therefore rechecks idle state immediately before sending the IPI, refraining from IPIing CPUs that have since gone idle.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Reported-by Rik van Riel <[email protected]>

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

atomic_add_return(0, ...) seems odd. Do you actually want that, rather than atomic_read(...)? If so, can you please document exactly why?

Yes that is weird. The only effective difference is that it would do a load-exclusive instead of a regular load.

It is weird, and checking to see if it is safe to convert it and its
friends to something with less overhead is on my list. This starts
with a patch series I will post soon that consolidates all these atomic_add_return() calls into a single function, which will ease testing and other verification.

Quoted 6 lines

All that aside, please keep in mind that much is required from this load. It is part of a network of ordered operations that guarantee that any operation from any CPU preceding a given grace period is seen to precede any other operation from any CPU following that same grace period. And each and every CPU must agree on the order of those two operations, otherwise, RCU is broken.

OK, so something similar to:

	smp_mb();
	atomic_read();

then? That would order, with global transitivity, against prior operations.

Quoted 2 lines

In addition, please note also that these operations are nowhere near any fastpaths.

My concern is mostly that it reads very weird. I appreciate this not being fast path code, but confusing code is bad in any form.

Hello!

This series contains miscellaneous fixes to RCU:

1. Tighten up __call_rcu's alignment check for the rcu_head structure
now that no arch does two-byte alignment of pointers.

2. Remove obsolete comment about when rcu_check_callbacks() was invoked.

3. Remove obsolete comment about which function opportunistically
notes grace periods beginnings and endings.

4. Update the RCU_TRACE Kconfig option's help text to note that
it enables event tracing in addition to debugfs, courtesy of Nikolay Borisov.

5. Add event tracing for long read-side delays in rcutorture
in order to better debug too-short grace periods.

6. Make expedited grace periods recheck dyntick-idle state to avoid
sending needless IPIs.

7. Don't kick CPUs unless there is grace-period activity currently
in progress.