rcu: Make __rcu_read_lock() inlinable

The preemptible-RCU implementations of __rcu_read_lock() have not been inlinable due to task_struct references that ran afoul of include-file dependencies. Fix this (as suggested by Linus) by moving the task_struct ->rcu_read_lock_nesting field to a per-CPU variable that is saved and restored at context-switch time. With this change, __rcu_read_lock() references only that new per-CPU variable, and so may be safely inlined. This change also allows some code that was duplicated in kernel/rcutree_plugin.h and kernel/rcutiny_plugin.h to be merged into include/linux/rcupdate.h.

This same approach unfortunately cannot be used on __rcu_read_unlock() because it also references the task_struct ->rcu_read_unlock_special field, to which cross-task access is required by rcu_boost(). This function will be handled in a separate commit, if need be.

The TREE_RCU variant passes modest rcutorture runs, while TINY_RCU still has a few bugs. Peter Zijlstra might have some thoughts on hooking into the scheduler. Disallows use of RCU from within the architecture-specific switch_to() function, which probably runs afoul of tracing for at least some architectures. There probably are still a few other bugs, as well.

TREE_RCU should be OK for experimental usage.

/*
+ * Save the incoming task's value for rcu_read_lock_nesting at the
+ * end of a context switch. There can be no process-state RCU read-side
+ * critical sections between the call to rcu_switch_from() and to
+ * rcu_switch_to(). Interrupt-level RCU read-side critical sections are
+ * OK because rcu_read_unlock_special() takes early exits when called
+ * at interrupt level.
+ */

Quoted 21 lines · Paul E. McKenney

The preemptible-RCU implementations of __rcu_read_lock() have not been inlinable due to task_struct references that ran afoul of include-file dependencies. Fix this (as suggested by Linus) by moving the task_struct ->rcu_read_lock_nesting field to a per-CPU variable that is saved and restored at context-switch time. With this change, __rcu_read_lock() references only that new per-CPU variable, and so may be safely inlined. This change also allows some code that was duplicated in kernel/rcutree_plugin.h and kernel/rcutiny_plugin.h to be merged into include/linux/rcupdate.h.

This same approach unfortunately cannot be used on __rcu_read_unlock() because it also references the task_struct ->rcu_read_unlock_special field, to which cross-task access is required by rcu_boost(). This function will be handled in a separate commit, if need be.

The TREE_RCU variant passes modest rcutorture runs, while TINY_RCU still has a few bugs. Peter Zijlstra might have some thoughts on hooking into the scheduler. Disallows use of RCU from within the architecture-specific switch_to() function, which probably runs afoul of tracing for at least some architectures. There probably are still a few other bugs, as well.

Quoted 1 line

TREE_RCU should be OK for experimental usage.

Right, so I really dislike adding this cache-miss to the context switch path, that said, rcu is used often enough that the savings on rcu_read_lock() might just come out in favour of this.. but it would be very nice to have some numbers.

Also,

Quoted 8 lines

/*
+ * Save the incoming task's value for rcu_read_lock_nesting at the
+ * end of a context switch. There can be no process-state RCU read-side
+ * critical sections between the call to rcu_switch_from() and to
+ * rcu_switch_to(). Interrupt-level RCU read-side critical sections are
+ * OK because rcu_read_unlock_special() takes early exits when called
+ * at interrupt level.
+ */

kernel/rcutiny_plugin.h void rcu_preempt_note_context_switch(void) · line 527

Quoted 7 lines

kernel/rcutiny_plugin.h +7 -0

+void rcu_switch_from(void)
+{
+	current->rcu_read_lock_nesting_save =
+		__this_cpu_read(rcu_read_lock_nesting);
+	barrier();
+	__this_cpu_write(rcu_read_lock_nesting, 0);
+}

Since rcu_switch_to() will again write rcu_read_lock_nesting, what's the point of setting it to zero?

Also, that barrier(), there's a clear dependency between the operations how can the compiler mess that up?

Quoted 6 lines

kernel/rcutiny_plugin.h +4 -0

+/*
+ * Restore the incoming task's value for rcu_read_lock_nesting at the
+ * end of a context switch.
  */
+void rcu_switch_to(void)
 {

kernel/rcutiny_plugin.h void rcu_preempt_note_context_switch(void) · line 539

Quoted 5 lines

kernel/rcutiny_plugin.h +4 -0

+	__this_cpu_write(rcu_read_lock_nesting,
+			 current->rcu_read_lock_nesting_save);
+	barrier();
+	current->rcu_read_lock_nesting_save = 0;
 }

Similar, a future rcu_switch_from() will again over-write current->rcu_read_lock_nesting_save, what's the point of clearing it?

kernel/sched/core.c context_switch(struct rq *rq, struct task_struct *prev, · line 2059

Quoted 12 lines

kernel/sched/core.c +2 -0

@@ -2051,7 +2051,9 @@ context_switch(struct rq *rq, struct task_struct *prev,
 #endif
 
 	/* Here we just switch the register state and the stack. */
+	rcu_switch_from();
 	switch_to(prev, next, prev);
+	rcu_switch_to();
 
 	barrier();
 	/*

So why not save one call and do:

	switch_to(prev, next, prev);

rcu_switch_to(prev, next);

and have

void rcu_switch_to(struct task_struct *prev, struct task_struct *next)

{
	prev->rcu_read_lock_nesting_save = __this_cpu_read(rcu_read_lock_nesting);
	__this_cpu_write(rcu_read_lock_nesting) = next->rcu_read_lock_nesting_save;
}

preferably as an inline function so we can avoid all calls.

Preemption is disabled at this point, correct?

Quoted 2 lines · Paul E. McKenney

Preemption is disabled at this point, correct?

Is this a philosophical question? You're asking if preemption is disabled while a task is currently being preempted ;-)

I could inline them into sched.h, if you are agreeable.

I am a bit concerned about putting them both together because I am betting that at least some of the architectures having tracing in switch_to(), which I currently do not handle well.

At the moment, the ways I can
think of to handle it well require saving before the switch and restoring afterwards. Otherwise, I can end up with the ->rcu_read_unlock_special flags getting associated with the wrong RCU read-side critical section, as happened last year.

Preemption is disabled at this point, correct?

Hmmm... One way that I could reduce the overhead that preemptible RCU imposes on the scheduler would be to move the task_struct queuing from its current point upon entry to the scheduler to just before switch_to(). (The _bh and _sched quiescent states still need to remain at scheduler entry.) That would mean that RCU would not queue tasks that entered the scheduler, but did not actually do a context switch.

Would that be helpful?

Quoted 2 lines · Paul E. McKenney

I could inline them into sched.h, if you are agreeable.

Sure, or put it in kernel/sched/core.c.

Quoted 3 lines

I am a bit concerned about putting them both together because I am betting that at least some of the architectures having tracing in switch_to(), which I currently do not handle well.

I would hope not.. there's a generic trace_sched_switch() and switch_to() is supposed to be black magic. I'd be fine breaking that as long as we can detect it.

Quoted 7 lines

At the moment, the ways I can
think of to handle it well require saving before the switch and restoring afterwards. Otherwise, I can end up with the ->rcu_read_unlock_special flags getting associated with the wrong RCU read-side critical section, as happened last year.

Preemption is disabled at this point, correct?

Yeah, and soon we'll have interrupts disabled as well (on all archs, currently only ARM has interrupts enabled at this point).

Quoted 6 lines

Hmmm... One way that I could reduce the overhead that preemptible RCU imposes on the scheduler would be to move the task_struct queuing from its current point upon entry to the scheduler to just before switch_to(). (The _bh and _sched quiescent states still need to remain at scheduler entry.) That would mean that RCU would not queue tasks that entered the scheduler, but did not actually do a context switch.

That would make sense anyhow, right? No point in queueing a task if you didn't actually switch away from it.

Quoted 1 line

Would that be helpful?

For now that's preemptible rcu only, and as such a somewhat niche feature (iirc its not enabled in the big distros) so I don't think it matters too much. But yeah, would be nice.

Hmmm... I am not yet sure whether it is easier to make RCU use legal in switch_to() or to detect it. I am inclined to take whatever course is easiest, which is likely to make it legal. :-/

Quoted 3 lines · Paul E. McKenney

Hmmm... I am not yet sure whether it is easier to make RCU use legal in switch_to() or to detect it. I am inclined to take whatever course is easiest, which is likely to make it legal. :-/

We could just declare that we do not allow tracepoints in arch specific "switch_to" code. Then you shouldn't need to worry about RCU in switch_to().

sched_rcu can still work there correct? That is, a synchronize_sched() should not be affected. As that is needed for the function tracing, and that may be called within a switch_to.

Quoted 8 lines · Paul E. McKenney

kernel/rcutree_plugin.h +5 -2

+void rcu_switch_from(void)
 {
-	current->rcu_read_lock_nesting++;
-	barrier();  /* needed if we ever invoke rcu_read_lock in rcutree.c */
+	current->rcu_read_lock_nesting_save =
+		__this_cpu_read(rcu_read_lock_nesting);
+	barrier();
+	__this_cpu_write(rcu_read_lock_nesting, 0);

(quoted diff) +1 -1

-	__this_cpu_write(rcu_read_lock_nesting, 0);
+	__this_cpu_write(rcu_read_lock_nesting, 1);

if prev or next task has non-zero rcu_read_unlock_special, "__this_cpu_write(rcu_read_lock_nesting, 1)" will prevent wrong qs reporting when rcu_read_unlock() is called in any interrupt/tracing while doing switch_to().

Quoted 13 lines

kernel/rcutree_plugin.h +12 -0

+}
+
+/*
+ * Restore the incoming task's value for rcu_read_lock_nesting at the
+ * end of a context switch.
+ */
+void rcu_switch_to(void)
+{
+	__this_cpu_write(rcu_read_lock_nesting,
+			 current->rcu_read_lock_nesting_save);
+	barrier();
+	current->rcu_read_lock_nesting_save = 0;
 }

(quoted diff) +0 -2

-	barrier();
-	current->rcu_read_lock_nesting_save = 0;

rcu_read_lock_nesting_save is set but not used before next set here, just remove it.

I don't like it hooks too much into scheduler.

Approaches:
0) stay using function call
1) hook into kbuild(https://lkml.org/lkml/2011/3/27/170,https://lkml.org/lkml/2011/3/27/171)
2) hook into scheduler(still need more works for rcu_read_unlock())
3) Add rcu_read_lock_nesting to thread_info like preempt_count
4) resolve header-file dependence

For me
3=4>1>2>0