rcu: v2 Inlinable preemptible rcu_read_lock() and rcu_read_unlock()

RFC PATCH In review Paul E. McKenney 2 participants 15 messages Apr 14, 2012 – Apr 15, 2012

Subsystems (5)

VFS Scheduler exec and binfmt User-mode Linux (UML) +1 more subsystems

lore

Share & tools

lore

Paul E. McKenney

author

2012-04-14 16:20 UTC #0 lore raw

cover rcu: v2 Inlinable preemptible rcu_read_lock() and rcu_read_unlock()

Hello!

This series is version two of the inlinable versions of preemptible RCU's __rcu_read_lock() and __rcu_read_unlock(). The first version may be found at https://lkml.org/lkml/2012/3/25/94. The individual commits in this new series are as follows:

1. Move preemptible RCU's hook in the scheduler from the common
RCU scheduler-entry hook to just before the scheduler's call to switch_to. This reduces overhead in the case where the scheduler is called but does not switch and also sets the stage for saving and restoring the per-CPU variables needed for inlining.

2. Create the per-CPU variables and rename rcu_read_unlock_special()
to avoid name conflict.

3. Make exit_rcu() use a more precise method of checking the need
for exit-time RCU-related cleanup, and consolidate the two identical versions of exit_rcu() into one place.

4. Make __rcu_read_lock() and __rcu_read_unlock() use the per-CPU
variables, but leave them out of line for the moment. This requires adding a second preemptible-RCU hook in the scheduler to restore the values of the per-CPU variables.

5. Silence bogus copy_to_user() build errors that seem to be triggered
by differences in gcc's inlining decisions when __rcu_read_lock() becomes inlinable. Apparently, copy_to_user() needs to be inlined in order to function correctly? Hmmm, sort of like kfree_rcu().

6. Inline __rcu_read_lock().

7. Inline __rcu_read_unlock().

With these changes, the 32-bit x86 gcc compiler compiles this:

	void rcu_read_lock_code(void)
	{
		rcu_read_lock();
	}

to this:

	000000d0 <rcu_read_lock_code>:
	  d0:	64 ff 05 00 00 00 00 	incl   %fs:0x0
	  d7:	c3                   	ret    
	  d8:	90                   	nop
	  d9:	8d b4 26 00 00 00 00 	lea    0x0(%esi,%eiz,1),%esi

It also compiles this:

	void rcu_read_unlock_code(void)
	{
		rcu_read_unlock();
	}

to this:

	000000e0 <rcu_read_unlock_code>:
	  e0:	64 a1 00 00 00 00    	mov    %fs:0x0,%eax
	  e6:	83 f8 01             	cmp    $0x1,%eax
	  e9:	74 0d                	je     f8 <rcu_read_unlock_code+0x18>
	  eb:	64 ff 0d 00 00 00 00 	decl   %fs:0x0
	  f2:	c3                   	ret    
	  f3:	90                   	nop
	  f4:	8d 74 26 00          	lea    0x0(%esi,%eiz,1),%esi
	  f8:	64 c7 05 00 00 00 00 	movl   $0x80000000,%fs:0x0
	  ff:	00 00 00 80 
	 103:	64 a1 00 00 00 00    	mov    %fs:0x0,%eax
	 109:	85 c0                	test   %eax,%eax
	 10b:	75 0c                	jne    119 <rcu_read_unlock_code+0x39>
	 10d:	64 c7 05 00 00 00 00 	movl   $0x0,%fs:0x0
	 114:	00 00 00 00 
	 118:	c3                   	ret    
	 119:	8d b4 26 00 00 00 00 	lea    0x0(%esi,%eiz,1),%esi
	 120:	e8 fc ff ff ff       	call   121 <rcu_read_unlock_code+0x41>
	 125:	eb e6                	jmp    10d <rcu_read_unlock_code+0x2d>

It is therefore not at all clear to me that the final patch in this series is worthwhile. Unless someone comes up with a good reason to keep it, I will drop it. The only possible justification I can see is that gcc could (in theory, anyway) drop dead code in the case of nested RCU read-side critical sections (everything from address f3 onwards), but this just doesn't cut it for me at the moment. I could also imagine having the inlined portion contain only the nesting check and decrement, along with a call to an out-of-line function that does the rest, but this looks to me to bloat the code for no good reason.

Thoughts?

Thanx, Paul

 arch/um/drivers/mconsole_kern.c   |    3 
 b/arch/um/drivers/mconsole_kern.c |    1 
 b/fs/binfmt_misc.c                |    4 -
 b/include/linux/init_task.h       |    4 -
 b/include/linux/rcupdate.h        |    1 
 b/include/linux/rcutiny.h         |    6 -
 b/include/linux/rcutree.h         |   12 ---
 b/include/linux/sched.h           |   10 +++
 b/kernel/rcu.h                    |    4 +
 b/kernel/rcupdate.c               |    5 +
 b/kernel/rcutiny_plugin.h         |   10 +--
 b/kernel/rcutree.c                |    1 
 b/kernel/rcutree.h                |    1 
 b/kernel/rcutree_plugin.h         |   14 ----
 b/kernel/sched/core.c             |    1 
 include/linux/rcupdate.h          |   72 ++++++++++++++++++++-
 include/linux/rcutiny.h           |    5 -
 include/linux/sched.h             |   92 +++++++++++++++++++++++++--
 kernel/rcu.h                      |    4 -
 kernel/rcupdate.c                 |  126 ++++++++++++++++++++++----------------
 kernel/rcutiny_plugin.h           |  123 +++++++------------------------------
 kernel/rcutree_plugin.h           |  114 ++++++++--------------------------
 kernel/sched/core.c               |    3 
 23 files changed, 321 insertions(+), 295 deletions(-)

Paul E. McKenney

author

2012-04-14 16:20 UTC #1 lore raw

RFC PATCH 1/7 rcu: Move PREEMPT_RCU preemption to switch_to() invocation

From: "Paul E. McKenney" <[email protected]>

Currently, PREEMPT_RCU readers are enqueued upon entry to the scheduler. This is inefficient because enqueuing is required only if there is a context switch, and entry to the scheduler does not guarantee a context switch.

The commit therefore moves the enqueuing to immediately precede the call to switch_to() from the scheduler.

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

---
 arch/um/drivers/mconsole_kern.c |    1 +
 include/linux/rcupdate.h        |    1 +
 include/linux/rcutiny.h         |    6 ------
 include/linux/sched.h           |   10 ++++++++++
 kernel/rcutree.c                |    1 -
 kernel/rcutree.h                |    1 -
 kernel/rcutree_plugin.h         |   14 +++-----------
 kernel/sched/core.c             |    1 +
 8 files changed, 16 insertions(+), 19 deletions(-)

arch/um/drivers/mconsole_kern.c +1 -0

index c70e047..f69ff2d 100644
@@ -704,6 +704,7 @@ static void stack_proc(void *arg)
 	struct task_struct *from = current, *to = arg;
 
 	to->thread.saved_task = from;
+	rcu_switch_from(from);
 	switch_to(from, to, from);
 }

include/linux/rcupdate.h +1 -0

index 9372174..aca4ef0 100644
@@ -183,6 +183,7 @@ static inline int rcu_preempt_depth(void)
 /* Internal to kernel */
 extern void rcu_sched_qs(int cpu);
 extern void rcu_bh_qs(int cpu);
+extern void rcu_preempt_note_context_switch(void);
 extern void rcu_check_callbacks(int cpu, int user);
 struct notifier_block;
 extern void rcu_idle_enter(void);

include/linux/rcutiny.h +0 -6

index e93df77..080b5bd 100644
@@ -87,10 +87,6 @@ static inline void kfree_call_rcu(struct rcu_head *head,
 
 #ifdef CONFIG_TINY_RCU
 
-static inline void rcu_preempt_note_context_switch(void)
-{
-}
-
 static inline void exit_rcu(void)
 {
 }

@@ -102,7 +98,6 @@ static inline int rcu_needs_cpu(int cpu)
 
 #else /* #ifdef CONFIG_TINY_RCU */
 
-void rcu_preempt_note_context_switch(void);
 extern void exit_rcu(void);
 int rcu_preempt_needs_cpu(void);

@@ -116,7 +111,6 @@ static inline int rcu_needs_cpu(int cpu)
 static inline void rcu_note_context_switch(int cpu)
 {
 	rcu_sched_qs(cpu);
-	rcu_preempt_note_context_switch();
 }
 
 /*

include/linux/sched.h +10 -0

index e692aba..a749c9d 100644
@@ -1879,12 +1879,22 @@ static inline void rcu_copy_process(struct task_struct *p)
 	INIT_LIST_HEAD(&p->rcu_node_entry);
 }
 
+static inline void rcu_switch_from(struct task_struct *prev)
+{
+	if (prev->rcu_read_lock_nesting != 0)
+		rcu_preempt_note_context_switch();
+}
+
 #else
 
 static inline void rcu_copy_process(struct task_struct *p)
 {
 }
 
+static inline void rcu_switch_from(struct task_struct *prev)
+{
+}
+
 #endif
 
 #ifdef CONFIG_SMP

kernel/rcutree.c +0 -1

index 1050d6d..6135150 100644
@@ -192,7 +192,6 @@ void rcu_note_context_switch(int cpu)
 {
 	trace_rcu_utilization("Start context switch");
 	rcu_sched_qs(cpu);
-	rcu_preempt_note_context_switch(cpu);
 	trace_rcu_utilization("End context switch");
 }
 EXPORT_SYMBOL_GPL(rcu_note_context_switch);

kernel/rcutree.h +0 -1

index cdd1be0..d6b70b08 100644
@@ -423,7 +423,6 @@ DECLARE_PER_CPU(char, rcu_cpu_has_work);
 /* Forward declarations for rcutree_plugin.h */
 static void rcu_bootup_announce(void);
 long rcu_batches_completed(void);
-static void rcu_preempt_note_context_switch(int cpu);
 static int rcu_preempt_blocked_readers_cgp(struct rcu_node *rnp);
 #ifdef CONFIG_HOTPLUG_CPU
 static void rcu_report_unblock_qs_rnp(struct rcu_node *rnp,

kernel/rcutree_plugin.h +3 ~~-11~~

index c023464..b1ac22e 100644
@@ -153,7 +153,7 @@ static void rcu_preempt_qs(int cpu)
  *
  * Caller must disable preemption.
  */
-static void rcu_preempt_note_context_switch(int cpu)
+void rcu_preempt_note_context_switch(void)
 {
 	struct task_struct *t = current;
 	unsigned long flags;

@@ -164,7 +164,7 @@ static void rcu_preempt_note_context_switch(int cpu)
 	    (t->rcu_read_unlock_special & RCU_READ_UNLOCK_BLOCKED) == 0) {
 
 		/* Possibly blocking in an RCU read-side critical section. */
-		rdp = per_cpu_ptr(rcu_preempt_state.rda, cpu);
+		rdp = __this_cpu_ptr(rcu_preempt_state.rda);
 		rnp = rdp->mynode;
 		raw_spin_lock_irqsave(&rnp->lock, flags);
 		t->rcu_read_unlock_special |= RCU_READ_UNLOCK_BLOCKED;

@@ -228,7 +228,7 @@ static void rcu_preempt_note_context_switch(int cpu)
 	 * means that we continue to block the current grace period.
 	 */
 	local_irq_save(flags);
-	rcu_preempt_qs(cpu);
+	rcu_preempt_qs(smp_processor_id());
 	local_irq_restore(flags);
 }

@@ -1018,14 +1018,6 @@ void rcu_force_quiescent_state(void)
 EXPORT_SYMBOL_GPL(rcu_force_quiescent_state);
 
 /*
- * Because preemptible RCU does not exist, we never have to check for
- * CPUs being in quiescent states.
- */
-static void rcu_preempt_note_context_switch(int cpu)
-{
-}
-
-/*
  * Because preemptible RCU does not exist, there are never any preempted
  * RCU readers.
  */

kernel/sched/core.c +1 -0

index 5255c9d..10689da 100644
@@ -2051,6 +2051,7 @@ context_switch(struct rq *rq, struct task_struct *prev,
 #endif
 
 	/* Here we just switch the register state and the stack. */
+	rcu_switch_from(prev);
 	switch_to(prev, next, prev);
 
 	barrier();

Signature · 1 line

1.7.8

Paul E. McKenney

author

2012-04-14 16:22 UTC #2 lore raw

RFC PATCH 2/7 rcu: Create per-CPU variables and avoid name conflict

From: "Paul E. McKenney" <[email protected]>

This commit creates the rcu_read_lock_nesting and rcu_read_unlock_special per-CPU variables, and renames the rcu_read_unlock_special() function to rcu_read_unlock_do_special() to avoid a name conflict with the new per-CPU variable.

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

---
 include/linux/rcupdate.h |    3 +++
 kernel/rcupdate.c        |    5 +++++
 kernel/rcutiny_plugin.h  |   10 +++++-----
 kernel/rcutree_plugin.h  |   12 ++++++------
 4 files changed, 19 insertions(+), 11 deletions(-)

include/linux/rcupdate.h +3 -0

index aca4ef0..1cf19ef 100644
@@ -144,6 +144,9 @@ extern void synchronize_sched(void);
 
 #ifdef CONFIG_PREEMPT_RCU
 
+DECLARE_PER_CPU(int, rcu_read_lock_nesting);
+DECLARE_PER_CPU(int, rcu_read_unlock_special);
+
 extern void __rcu_read_lock(void);
 extern void __rcu_read_unlock(void);
 void synchronize_rcu(void);

kernel/rcupdate.c +5 -0

index a86f174..eb5d160 100644
@@ -51,6 +51,11 @@
 
 #include "rcu.h"
 
+#ifdef CONFIG_PREEMPT_RCU
+DEFINE_PER_CPU(int, rcu_read_lock_nesting);
+DEFINE_PER_CPU(int, rcu_read_unlock_special);
+#endif /* #ifdef CONFIG_PREEMPT_RCU */
+
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 static struct lock_class_key rcu_lock_key;
 struct lockdep_map rcu_lock_map =

kernel/rcutiny_plugin.h +5 -5

index 22ecea0..ff7ec65 100644
@@ -132,7 +132,7 @@ static struct rcu_preempt_ctrlblk rcu_preempt_ctrlblk = {
 	RCU_TRACE(.rcb.name = "rcu_preempt")
 };
 
-static void rcu_read_unlock_special(struct task_struct *t);
+static void rcu_read_unlock_do_special(struct task_struct *t);
 static int rcu_preempted_readers_exp(void);
 static void rcu_report_exp_done(void);

@@ -510,7 +510,7 @@ void rcu_preempt_note_context_switch(void)
 		 * Complete exit from RCU read-side critical section on
 		 * behalf of preempted instance of __rcu_read_unlock().
 		 */
-		rcu_read_unlock_special(t);
+		rcu_read_unlock_do_special(t);
 	}
 
 	/*

@@ -543,7 +543,7 @@ EXPORT_SYMBOL_GPL(__rcu_read_lock);
  * notify RCU core processing or task having blocked during the RCU
  * read-side critical section.
  */
-static noinline void rcu_read_unlock_special(struct task_struct *t)
+static noinline void rcu_read_unlock_do_special(struct task_struct *t)
 {
 	int empty;
 	int empty_exp;

@@ -630,7 +630,7 @@ static noinline void rcu_read_unlock_special(struct task_struct *t)
  * Tiny-preemptible RCU implementation for rcu_read_unlock().
  * Decrement ->rcu_read_lock_nesting.  If the result is zero (outermost
  * rcu_read_unlock()) and ->rcu_read_unlock_special is non-zero, then
- * invoke rcu_read_unlock_special() to clean up after a context switch
+ * invoke rcu_read_unlock_do_special() to clean up after a context switch
  * in an RCU read-side critical section and other special cases.
  */
 void __rcu_read_unlock(void)

@@ -644,7 +644,7 @@ void __rcu_read_unlock(void)
 		t->rcu_read_lock_nesting = INT_MIN;
 		barrier();  /* assign before ->rcu_read_unlock_special load */
 		if (unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
-			rcu_read_unlock_special(t);
+			rcu_read_unlock_do_special(t);
 		barrier();  /* ->rcu_read_unlock_special load before assign */
 		t->rcu_read_lock_nesting = 0;
 	}

kernel/rcutree_plugin.h +6 -6

index b1ac22e..f60b315 100644
@@ -78,7 +78,7 @@ struct rcu_state rcu_preempt_state = RCU_STATE_INITIALIZER(rcu_preempt);
 DEFINE_PER_CPU(struct rcu_data, rcu_preempt_data);
 static struct rcu_state *rcu_state = &rcu_preempt_state;
 
-static void rcu_read_unlock_special(struct task_struct *t);
+static void rcu_read_unlock_do_special(struct task_struct *t);
 static int rcu_preempted_readers_exp(struct rcu_node *rnp);
 
 /*

@@ -215,7 +215,7 @@ void rcu_preempt_note_context_switch(void)
 		 * Complete exit from RCU read-side critical section on
 		 * behalf of preempted instance of __rcu_read_unlock().
 		 */
-		rcu_read_unlock_special(t);
+		rcu_read_unlock_do_special(t);
 	}
 
 	/*

@@ -310,7 +310,7 @@ static struct list_head *rcu_next_node_entry(struct task_struct *t,
  * notify RCU core processing or task having blocked during the RCU
  * read-side critical section.
  */
-static noinline void rcu_read_unlock_special(struct task_struct *t)
+static noinline void rcu_read_unlock_do_special(struct task_struct *t)
 {
 	int empty;
 	int empty_exp;

@@ -422,7 +422,7 @@ static noinline void rcu_read_unlock_special(struct task_struct *t)
  * Tree-preemptible RCU implementation for rcu_read_unlock().
  * Decrement ->rcu_read_lock_nesting.  If the result is zero (outermost
  * rcu_read_unlock()) and ->rcu_read_unlock_special is non-zero, then
- * invoke rcu_read_unlock_special() to clean up after a context switch
+ * invoke rcu_read_unlock_do_special() to clean up after a context switch
  * in an RCU read-side critical section and other special cases.
  */
 void __rcu_read_unlock(void)

@@ -436,7 +436,7 @@ void __rcu_read_unlock(void)
 		t->rcu_read_lock_nesting = INT_MIN;
 		barrier();  /* assign before ->rcu_read_unlock_special load */
 		if (unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
-			rcu_read_unlock_special(t);
+			rcu_read_unlock_do_special(t);
 		barrier();  /* ->rcu_read_unlock_special load before assign */
 		t->rcu_read_lock_nesting = 0;
 	}

@@ -573,7 +573,7 @@ static void rcu_preempt_check_blocked_tasks(struct rcu_node *rnp)
  * Handle tasklist migration for case in which all CPUs covered by the
  * specified rcu_node have gone offline.  Move them up to the root
  * rcu_node.  The reason for not just moving them to the immediate
- * parent is to remove the need for rcu_read_unlock_special() to
+ * parent is to remove the need for rcu_read_unlock_do_special() to
  * make more than two attempts to acquire the target rcu_node's lock.
  * Returns true if there were tasks blocking the current RCU grace
  * period.

Signature · 1 line

1.7.8

Paul E. McKenney

author

2012-04-14 16:20 UTC #3 lore raw

RFC PATCH 3/7 rcu: Make exit_rcu() more precise and consolidate

In reply to Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

When running preemptible RCU, if a task exits in an RCU read-side critical section having blocked within that same RCU read-side critical section, the task must be removed from the list of tasks blocking a grace period (perhaps the current grace period, perhaps the next grace period, depending on timing). The exit() path invokes exit_rcu() to do this cleanup.

However, the current implementation of exit_rcu() needlessly does the cleanup even if the task did not block within the current RCU read-side critical section, which wastes time and needlessly increases the size of the state space. Fix this by only doing the cleanup if the current task is actually on the list of tasks blocking some grace period.

While we are at it, consolidate the two identical exit_rcu() functions into a single function.

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

---
 include/linux/rcupdate.h |    1 +
 include/linux/rcutiny.h  |    5 -----
 include/linux/rcutree.h  |   12 ------------
 kernel/rcupdate.c        |   27 ++++++++++++++++++++++++++-
 kernel/rcutiny_plugin.h  |   16 ----------------
 kernel/rcutree_plugin.h  |   16 ----------------
 6 files changed, 27 insertions(+), 50 deletions(-)

include/linux/rcupdate.h +1 -0

index 1cf19ef..5a94dcf 100644
@@ -193,6 +193,7 @@ extern void rcu_idle_enter(void);
 extern void rcu_idle_exit(void);
 extern void rcu_irq_enter(void);
 extern void rcu_irq_exit(void);
+extern void exit_rcu(void);
 
 /**
  * RCU_NONIDLE - Indicate idle-loop code that needs RCU readers

include/linux/rcutiny.h +0 -5

index 080b5bd..adb5e5a 100644
@@ -87,10 +87,6 @@ static inline void kfree_call_rcu(struct rcu_head *head,
 
 #ifdef CONFIG_TINY_RCU
 
-static inline void exit_rcu(void)
-{
-}
-
 static inline int rcu_needs_cpu(int cpu)
 {
 	return 0;

@@ -98,7 +94,6 @@ static inline int rcu_needs_cpu(int cpu)
 
 #else /* #ifdef CONFIG_TINY_RCU */
 
-extern void exit_rcu(void);
 int rcu_preempt_needs_cpu(void);
 
 static inline int rcu_needs_cpu(int cpu)

include/linux/rcutree.h +0 ~~-12~~

index e8ee5dd..782a8ab 100644
@@ -45,18 +45,6 @@ static inline void rcu_virt_note_context_switch(int cpu)
 	rcu_note_context_switch(cpu);
 }
 
-#ifdef CONFIG_TREE_PREEMPT_RCU
-
-extern void exit_rcu(void);
-
-#else /* #ifdef CONFIG_TREE_PREEMPT_RCU */
-
-static inline void exit_rcu(void)
-{
-}
-
-#endif /* #else #ifdef CONFIG_TREE_PREEMPT_RCU */
-
 extern void synchronize_rcu_bh(void);
 extern void synchronize_sched_expedited(void);
 extern void synchronize_rcu_expedited(void);

kernel/rcupdate.c +26 -1

index eb5d160..1acf0da 100644
@@ -54,7 +54,32 @@
 #ifdef CONFIG_PREEMPT_RCU
 DEFINE_PER_CPU(int, rcu_read_lock_nesting);
 DEFINE_PER_CPU(int, rcu_read_unlock_special);
-#endif /* #ifdef CONFIG_PREEMPT_RCU */
+
+/*
+ * Check for a task exiting while in a preemptible-RCU read-side
+ * critical section, clean up if so.  No need to issue warnings,
+ * as debug_check_no_locks_held() already does this if lockdep
+ * is enabled.
+ */
+void exit_rcu(void)
+{
+	struct task_struct *t = current;
+
+	if (likely(list_empty(&current->rcu_node_entry)))
+		return;
+	t->rcu_read_lock_nesting = 1;
+	barrier();
+	t->rcu_read_unlock_special = RCU_READ_UNLOCK_BLOCKED;
+	__rcu_read_unlock();
+}
+
+#else /* #ifdef CONFIG_PREEMPT_RCU */
+
+void exit_rcu(void)
+{
+}
+
+#endif /* #else #ifdef CONFIG_PREEMPT_RCU */
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 static struct lock_class_key rcu_lock_key;

kernel/rcutiny_plugin.h +0 ~~-16~~

index ff7ec65..66a542a 100644
@@ -851,22 +851,6 @@ int rcu_preempt_needs_cpu(void)
 	return rcu_preempt_ctrlblk.rcb.rcucblist != NULL;
 }
 
-/*
- * Check for a task exiting while in a preemptible -RCU read-side
- * critical section, clean up if so.  No need to issue warnings,
- * as debug_check_no_locks_held() already does this if lockdep
- * is enabled.
- */
-void exit_rcu(void)
-{
-	struct task_struct *t = current;
-
-	if (t->rcu_read_lock_nesting == 0)
-		return;
-	t->rcu_read_lock_nesting = 1;
-	__rcu_read_unlock();
-}
-
 #else /* #ifdef CONFIG_TINY_PREEMPT_RCU */
 
 #ifdef CONFIG_RCU_TRACE

kernel/rcutree_plugin.h +0 ~~-16~~

index f60b315..76a1ba9 100644
@@ -969,22 +969,6 @@ static void __init __rcu_init_preempt(void)
 	rcu_init_one(&rcu_preempt_state, &rcu_preempt_data);
 }
 
-/*
- * Check for a task exiting while in a preemptible-RCU read-side
- * critical section, clean up if so.  No need to issue warnings,
- * as debug_check_no_locks_held() already does this if lockdep
- * is enabled.
- */
-void exit_rcu(void)
-{
-	struct task_struct *t = current;
-
-	if (t->rcu_read_lock_nesting == 0)
-		return;
-	t->rcu_read_lock_nesting = 1;
-	__rcu_read_unlock();
-}
-
 #else /* #ifdef CONFIG_TREE_PREEMPT_RCU */
 
 static struct rcu_state *rcu_state = &rcu_sched_state;

Signature · 1 line

1.7.8

Paul E. McKenney

author

2012-04-14 16:21 UTC #4 lore raw

RFC PATCH 4/7 rcu: Move __rcu_read_lock() and __rcu_read_unlock() to per-CPU variables

In reply to Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

This commit is another step towards inlinable __rcu_read_lock() and __rcu_read_unlock() functions for preemptible RCU. This keeps these two functions out of line, but switches them to use the per-CPU variables that are required to export their definitions without requiring that all RCU users include sched.h. These per-CPU variables are saved and restored at context-switch time.

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

---
 arch/um/drivers/mconsole_kern.c |    3 +-
 include/linux/init_task.h       |    4 +-
 include/linux/rcupdate.h        |   20 +++++++-
 include/linux/sched.h           |   92 +++++++++++++++++++++++++++++++---
 kernel/rcu.h                    |    4 ++
 kernel/rcupdate.c               |   54 ++++++++++++++++++--
 kernel/rcutiny_plugin.h         |  106 ++++++++------------------------------
 kernel/rcutree_plugin.h         |   85 ++++++++-----------------------
 kernel/sched/core.c             |    3 +-
 9 files changed, 209 insertions(+), 162 deletions(-)

arch/um/drivers/mconsole_kern.c +2 -1

index f69ff2d..a98693f 100644
@@ -704,8 +704,9 @@ static void stack_proc(void *arg)
 	struct task_struct *from = current, *to = arg;
 
 	to->thread.saved_task = from;
-	rcu_switch_from(from);
+	rcu_switch_from();
 	switch_to(from, to, from);
+	rcu_switch_to();
 }
 
 /*

include/linux/init_task.h +2 -2

index 9c66b1a..cdd8436 100644
@@ -105,8 +105,8 @@ extern struct group_info init_groups;
 #endif
 #ifdef CONFIG_PREEMPT_RCU
 #define INIT_TASK_RCU_PREEMPT(tsk)					\
-	.rcu_read_lock_nesting = 0,					\
-	.rcu_read_unlock_special = 0,					\
+	.rcu_read_lock_nesting_save = 0,				\
+	.rcu_read_unlock_special_save = 0,				\
 	.rcu_node_entry = LIST_HEAD_INIT(tsk.rcu_node_entry),		\
 	INIT_TASK_RCU_TREE_PREEMPT()					\
 	INIT_TASK_RCU_BOOST()

include/linux/rcupdate.h +19 -1

index 5a94dcf..89f7e97 100644
@@ -146,6 +146,9 @@ extern void synchronize_sched(void);
 
 DECLARE_PER_CPU(int, rcu_read_lock_nesting);
 DECLARE_PER_CPU(int, rcu_read_unlock_special);
+#ifdef CONFIG_PROVE_RCU
+DECLARE_PER_CPU(struct task_struct *, rcu_current_task);
+#endif /* #ifdef CONFIG_PROVE_RCU */
 
 extern void __rcu_read_lock(void);
 extern void __rcu_read_unlock(void);

@@ -157,7 +160,22 @@ void synchronize_rcu(void);
  * nesting depth, but makes sense only if CONFIG_PREEMPT_RCU -- in other
  * types of kernel builds, the rcu_read_lock() nesting depth is unknowable.
  */
-#define rcu_preempt_depth() (current->rcu_read_lock_nesting)
+#define rcu_preempt_depth() (__this_cpu_read(rcu_read_lock_nesting))
+
+/*
+ * Check for a running RCU reader on the current CPU.  If used from
+ * TINY_PREEMPT_RCU, works globally, as there can be but one running
+ * RCU reader at a time in that case.  ;-)
+ *
+ * Returns zero if there are no running readers.  Returns a positive
+ * number if there is at least one reader within its RCU read-side
+ * critical section.  Returns a negative number if an outermost reader
+ * is in the midst of exiting from its RCU read-side critical section
+ *
+ * This differs from rcu_preempt_depth() in throwing a build error
+ * if used from under !CONFIG_PREEMPT_RCU.
+ */
+#define rcu_preempt_running_reader() (__this_cpu_read(rcu_read_lock_nesting))
 
 #else /* #ifdef CONFIG_PREEMPT_RCU */

include/linux/sched.h +85 -7

118 lines not on this page · raw message

kernel/rcu.h +4 -0

index 8ba99cd..6243d8d 100644
@@ -109,4 +109,8 @@ static inline bool __rcu_reclaim(char *rn, struct rcu_head *head)
 	}
 }
 
+#ifdef CONFIG_PREEMPT_RCU
+extern void rcu_read_unlock_do_special(void);
+#endif /* #ifdef CONFIG_PREEMPT_RCU */
+
 #endif /* __LINUX_RCU_H */

kernel/rcupdate.c +50 -4

69 lines not on this page · raw message

kernel/rcutiny_plugin.h +23 ~~-83~~

192 lines not on this page · raw message

kernel/rcutree_plugin.h +22 ~~-63~~

163 lines not on this page · raw message

kernel/sched/core.c +2 -1

index 10689da..9f10b76 100644
@@ -2051,8 +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(prev);
+	rcu_switch_from();
 	switch_to(prev, next, prev);
+	rcu_switch_to();
 
 	barrier();
 	/*

Signature · 1 line

1.7.8

Paul E. McKenney

author

2012-04-14 16:20 UTC #5 lore raw

RFC PATCH 5/7 fs: Silence bogus copy_to_user() build errors

In reply to Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

The copy_to_user() function now does some compile-time buffer-size checks, but these checks can be fooled by pointers, which always appear to be 4 or 8 bytes long for 32-bit and 64-bit builds, respectively. Take care of the warnings in fs/binfmt_misc.c by invoking the underlying _copy_to_user() function.

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

---
 fs/binfmt_misc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

fs/binfmt_misc.c +2 -2

index a9198df..e5ea8c3 100644
@@ -310,7 +310,7 @@ static Node *create_entry(const char __user *buffer, size_t count)
 	p = buf = (char *)e + sizeof(Node);
 
 	memset(e, 0, sizeof(Node));
-	if (copy_from_user(buf, buffer, count))
+	if (_copy_from_user(buf, buffer, count))
 		goto Efault;
 
 	del = *p++;	/* delimeter */

@@ -418,7 +418,7 @@ static int parse_command(const char __user *buffer, size_t count)
 		return 0;
 	if (count > 3)
 		return -EINVAL;
-	if (copy_from_user(s, buffer, count))
+	if (_copy_from_user(s, buffer, count))
 		return -EFAULT;
 	if (s[count-1] == '\n')
 		count--;

Signature · 1 line

1.7.8

Paul E. McKenney

author

2012-04-14 16:21 UTC #6 lore raw

RFC PATCH 6/7 rcu: Inline preemptible RCU __rcu_read_lock()

In reply to Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Move __rcu_read_lock() from kernel/rcupdate.c to include/linux/rcupdate.h, allowing the compiler to inline it.

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

---
 include/linux/rcupdate.h |   13 ++++++++++++-
 kernel/rcupdate.c        |   12 ------------
 2 files changed, 12 insertions(+), 13 deletions(-)

include/linux/rcupdate.h +12 -1

index 89f7e97..9967b2b 100644
@@ -43,6 +43,7 @@
 #include <linux/completion.h>
 #include <linux/debugobjects.h>
 #include <linux/compiler.h>
+#include <linux/percpu.h>
 
 #ifdef CONFIG_RCU_TORTURE_TEST
 extern int rcutorture_runnable; /* for sysctl */

@@ -150,7 +151,17 @@ DECLARE_PER_CPU(int, rcu_read_unlock_special);
 DECLARE_PER_CPU(struct task_struct *, rcu_current_task);
 #endif /* #ifdef CONFIG_PROVE_RCU */
 
-extern void __rcu_read_lock(void);
+/*
+ * Preemptible-RCU implementation for rcu_read_lock().  Just increment
+ * the per-CPU rcu_read_lock_nesting: Shared state and per-task state will
+ * be updated if we block.
+ */
+static inline void __rcu_read_lock(void)
+{
+	__this_cpu_inc(rcu_read_lock_nesting);
+	barrier(); /* Keep code within RCU read-side critical section. */
+}
+
 extern void __rcu_read_unlock(void);
 void synchronize_rcu(void);

kernel/rcupdate.c +0 ~~-12~~

index f77a5fc..d52c68e 100644
@@ -59,18 +59,6 @@ DEFINE_PER_CPU(struct task_struct *, rcu_current_task);
 #endif /* #ifdef CONFIG_PROVE_RCU */
 
 /*
- * Preemptible-RCU implementation for rcu_read_lock().  Just increment
- * the per-CPU rcu_read_lock_nesting: Shared state and per-task state will
- * be updated if we block.
- */
-void __rcu_read_lock(void)
-{
-	__this_cpu_inc(rcu_read_lock_nesting);
-	barrier(); /* Keep code within RCU read-side critical section. */
-}
-EXPORT_SYMBOL_GPL(__rcu_read_lock);
-
-/*
  * Tree-preemptible RCU implementation for rcu_read_unlock().
  * Decrement rcu_read_lock_nesting.  If the result is zero (outermost
  * rcu_read_unlock()) and rcu_read_unlock_special is non-zero, then

Signature · 1 line

1.7.8

Paul E. McKenney

author

2012-04-14 16:20 UTC #7 lore raw

RFC PATCH 7/7 rcu: Inline preemptible RCU __rcu_read_unlock()

In reply to Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Move __rcu_read_unlock() from kernel/rcupdate.c to include/linux/rcupdate.h, allowing the compiler to inline it.

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

---
 include/linux/rcupdate.h |   35 ++++++++++++++++++++++++++++++++++-
 kernel/rcu.h             |    4 ----
 kernel/rcupdate.c        |   33 ---------------------------------
 kernel/rcutiny_plugin.h  |    1 +
 kernel/rcutree_plugin.h  |    1 +
 5 files changed, 36 insertions(+), 38 deletions(-)

include/linux/rcupdate.h +34 -1

index 9967b2b..8113505 100644
@@ -162,7 +162,40 @@ static inline void __rcu_read_lock(void)
 	barrier(); /* Keep code within RCU read-side critical section. */
 }
 
-extern void __rcu_read_unlock(void);
+extern void rcu_read_unlock_do_special(void);
+
+/*
+ * Tree-preemptible RCU implementation for rcu_read_unlock().
+ * Decrement rcu_read_lock_nesting.  If the result is zero (outermost
+ * rcu_read_unlock()) and rcu_read_unlock_special is non-zero, then
+ * invoke rcu_read_unlock_do_special() to clean up after a context switch
+ * in an RCU read-side critical section and other special cases.
+ * Set rcu_read_lock_nesting to a large negative value during cleanup
+ * in order to ensure that if rcu_read_unlock_special is non-zero, then
+ * rcu_read_lock_nesting is also non-zero.
+ */
+static inline void __rcu_read_unlock(void)
+{
+	if (__this_cpu_read(rcu_read_lock_nesting) != 1)
+		__this_cpu_dec(rcu_read_lock_nesting);
+	else {
+		barrier();  /* critical section before exit code. */
+		__this_cpu_write(rcu_read_lock_nesting, INT_MIN);
+		barrier();  /* assign before ->rcu_read_unlock_special load */
+		if (unlikely(__this_cpu_read(rcu_read_unlock_special)))
+			rcu_read_unlock_do_special();
+		barrier();  /* ->rcu_read_unlock_special load before assign */
+		__this_cpu_write(rcu_read_lock_nesting, 0);
+	}
+#ifdef CONFIG_PROVE_LOCKING
+	{
+		int rln = __this_cpu_read(rcu_read_lock_nesting);
+
+		WARN_ON_ONCE(rln < 0 && rln > INT_MIN / 2);
+	}
+#endif /* #ifdef CONFIG_PROVE_LOCKING */
+}
+
 void synchronize_rcu(void);
 
 /*

kernel/rcu.h +0 -4

index 6243d8d..8ba99cd 100644
@@ -109,8 +109,4 @@ static inline bool __rcu_reclaim(char *rn, struct rcu_head *head)
 	}
 }
 
-#ifdef CONFIG_PREEMPT_RCU
-extern void rcu_read_unlock_do_special(void);
-#endif /* #ifdef CONFIG_PREEMPT_RCU */
-
 #endif /* __LINUX_RCU_H */

kernel/rcupdate.c +0 ~~-33~~

index d52c68e..f607cb5 100644
@@ -59,39 +59,6 @@ DEFINE_PER_CPU(struct task_struct *, rcu_current_task);
 #endif /* #ifdef CONFIG_PROVE_RCU */
 
 /*
- * Tree-preemptible RCU implementation for rcu_read_unlock().
- * Decrement rcu_read_lock_nesting.  If the result is zero (outermost
- * rcu_read_unlock()) and rcu_read_unlock_special is non-zero, then
- * invoke rcu_read_unlock_do_special() to clean up after a context switch
- * in an RCU read-side critical section and other special cases.
- * Set rcu_read_lock_nesting to a large negative value during cleanup
- * in order to ensure that if rcu_read_unlock_special is non-zero, then
- * rcu_read_lock_nesting is also non-zero.
- */
-void __rcu_read_unlock(void)
-{
-	if (__this_cpu_read(rcu_read_lock_nesting) != 1)
-		__this_cpu_dec(rcu_read_lock_nesting);
-	else {
-		barrier();  /* critical section before exit code. */
-		__this_cpu_write(rcu_read_lock_nesting, INT_MIN);
-		barrier();  /* assign before ->rcu_read_unlock_special load */
-		if (unlikely(__this_cpu_read(rcu_read_unlock_special)))
-			rcu_read_unlock_do_special();
-		barrier();  /* ->rcu_read_unlock_special load before assign */
-		__this_cpu_write(rcu_read_lock_nesting, 0);
-	}
-#ifdef CONFIG_PROVE_LOCKING
-	{
-		int rln = __this_cpu_read(rcu_read_lock_nesting);
-
-		WARN_ON_ONCE(rln < 0 && rln > INT_MIN / 2);
-	}
-#endif /* #ifdef CONFIG_PROVE_LOCKING */
-}
-EXPORT_SYMBOL_GPL(__rcu_read_unlock);
-
-/*
  * Check for a task exiting while in a preemptible-RCU read-side
  * critical section, clean up if so.  No need to issue warnings,
  * as debug_check_no_locks_held() already does this if lockdep

kernel/rcutiny_plugin.h +1 -0

index 6b416af..49cb5b0 100644
@@ -598,6 +598,7 @@ void rcu_read_unlock_do_special(void)
 	}
 	local_irq_restore(flags);
 }
+EXPORT_SYMBOL_GPL(rcu_read_unlock_do_special);
 
 /*
  * Check for a quiescent state from the current CPU.  When a task blocks,

kernel/rcutree_plugin.h +1 -0

index 20be289..7afde96 100644
@@ -409,6 +409,7 @@ void rcu_read_unlock_do_special(void)
 		local_irq_restore(flags);
 	}
 }
+EXPORT_SYMBOL_GPL(rcu_read_unlock_do_special);
 
 #ifdef CONFIG_RCU_CPU_STALL_VERBOSE

Signature · 1 line

1.7.8

Linus Torvalds

2012-04-14 16:28 UTC #8 lore raw

In reply to Paul E. McKenney

Quoted 30 lines · Paul E. McKenney

It also compiles this:
void rcu_read_unlock_code(void)
{
rcu_read_unlock();
}
to this:
000000e0 <rcu_read_unlock_code>:
e0: 64 a1 00 00 00 00 mov %fs:0x0,%eax
e6: 83 f8 01 cmp $0x1,%eax
e9: 74 0d je f8 <rcu_read_unlock_code+0x18>
eb: 64 ff 0d 00 00 00 00 decl %fs:0x0
f2: c3 ret
f3: 90 nop
f4: 8d 74 26 00 lea 0x0(%esi,%eiz,1),%esi
f8: 64 c7 05 00 00 00 00 movl $0x80000000,%fs:0x0
ff: 00 00 00 80
103: 64 a1 00 00 00 00 mov %fs:0x0,%eax
109: 85 c0 test %eax,%eax
10b: 75 0c jne 119 <rcu_read_unlock_code+0x39> 10d: 64 c7 05 00 00 00 00 movl $0x0,%fs:0x0
114: 00 00 00 00
118: c3 ret
119: 8d b4 26 00 00 00 00 lea 0x0(%esi,%eiz,1),%esi 120: e8 fc ff ff ff call 121 <rcu_read_unlock_code+0x41> 125: eb e6 jmp 10d <rcu_read_unlock_code+0x2d>

Quoted 2 lines

It is therefore not at all clear to me that the final patch in this series is worthwhile.

Yeah, the read-unlock does not seem to be worth inlining as-is. As mentioned earlier, it tends to be the rcu_read_lock() that we really want to inline anyway (not just because it's small, but also because the call clobbers registers, and that has other bad interaction).

Linus

Linus Torvalds

2012-04-14 16:39 UTC #9 lore raw

In reply to Paul E. McKenney

Quoted 3 lines · Paul E. McKenney

This series is version two of the inlinable versions of preemptible RCU's __rcu_read_lock() and __rcu_read_unlock().

Btw, what is this supposed to apply on top of? It doesn't seem to apply cleanly to either v3.3, v3.4-rc[12], or current top-of-tree.

I can make it apply by reducing the context to just one line for patch#6, but your base seems to be odd.

Linus

Paul E. McKenney

author

2012-04-14 16:52 UTC #10 lore raw

Quoted 11 lines · Linus Torvalds

Quoted 3 lines · Paul E. McKenney
This series is version two of the inlinable versions of preemptible RCU's __rcu_read_lock() and __rcu_read_unlock().
Btw, what is this supposed to apply on top of? It doesn't seem to apply cleanly to either v3.3, v3.4-rc[12], or current top-of-tree.
I can make it apply by reducing the context to just one line for patch#6, but your base seems to be odd.

My next step is to forward-port to 4.4-rc2, retest, and expose to -next. The base is on top of the series that was pulled into the 4.4 merge window, but on their base on top of 3.4-rc4.

I probably should have forward-ported before submitting, but I was so happy about it finally actually passing a moderate version of the full set of rcutorture tests that I just couldn't restrain myself.

Thanx, Paul

Linus Torvalds

2012-04-14 16:58 UTC #11 lore raw

In reply to Paul E. McKenney

Quoted 5 lines · Paul E. McKenney

This series is version two of the inlinable versions of preemptible RCU's __rcu_read_lock() and __rcu_read_unlock(). The first version may be found at https://lkml.org/lkml/2012/3/25/94. The individual commits in this new series are as follows:

Ok, I'm looking at code generation changes for this now, and sadly (or not) all the changes to the selinux code have actually made the RCU overhead that made me notice this issue originally much less noticeable because we inline the right things etc, and that helped with some of the problems that the rcu callsites had.

The fact that we don't have to indirect through "current" does seem to make the code much simpler, though. So hopefully the actual rcu unlocking code itself is better. But that was just under 1% of the benchmark cost, so I was hoping that the call sites would be more noticeable.

I'll see if it boots and what it does to my profiles and microbenchmark, though.

Linus

Linus Torvalds

2012-04-14 17:09 UTC #12 lore raw

Quoted 3 lines · Linus Torvalds

I'll see if it boots and what it does to my profiles and microbenchmark, though.

Ok, I can't see any performance difference in the numbers - my benchmark noise is *much* bigger than anything this would show.

The profile looks fine, and obviously __rcu_read_lock() is entirely gone. The top user (avc_has_perm_flags()) looks fine. I note that you might want to look at the placement of the percpu data - I think it probably makes sense to put the RCU data close to 'current' etc to get as much cacheline sharing as possible, and it doesn't seem to be right now, but it looks reasonable.

But on the whole, I can't claim that it looks noticeable ;*(

Linus

Paul E. McKenney

author

2012-04-14 17:26 UTC #13 lore raw

Quoted 8 lines · Linus Torvalds

Quoted 3 lines · Linus Torvalds
I'll see if it boots and what it does to my profiles and microbenchmark, though.
Ok, I can't see any performance difference in the numbers - my benchmark noise is *much* bigger than anything this would show.

Might still be worthwhile on embedded CPUs that don't optimize function calls as thoroughly as does x86, maybe?

Quoted 6 lines

The profile looks fine, and obviously __rcu_read_lock() is entirely gone. The top user (avc_has_perm_flags()) looks fine. I note that you might want to look at the placement of the percpu data - I think it probably makes sense to put the RCU data close to 'current' etc to get as much cacheline sharing as possible, and it doesn't seem to be right now, but it looks reasonable.

Is there somewhere in non-architecture-specific code that would be a good place to put this? Or is the DEFINE_PER_CPU() for current_task moving from arch/x86 to core code?

Quoted 1 line

But on the whole, I can't claim that it looks noticeable ;*(

Well, then, I guess I don't feel quite so bad about having prioritized this so low for so long. ;-)

Thanx, Paul

Paul E. McKenney

author

2012-04-15 16:26 UTC #14 lore raw

Quoted 20 lines · Paul E. McKenney

Quoted 8 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
I'll see if it boots and what it does to my profiles and microbenchmark, though.
Ok, I can't see any performance difference in the numbers - my benchmark noise is *much* bigger than anything this would show.
Might still be worthwhile on embedded CPUs that don't optimize function calls as thoroughly as does x86, maybe?
Quoted 6 lines
The profile looks fine, and obviously __rcu_read_lock() is entirely gone. The top user (avc_has_perm_flags()) looks fine. I note that you might want to look at the placement of the percpu data - I think it probably makes sense to put the RCU data close to 'current' etc to get as much cacheline sharing as possible, and it doesn't seem to be right now, but it looks reasonable.

Quoted 8 lines

Is there somewhere in non-architecture-specific code that would be a good place to put this? Or is the DEFINE_PER_CPU() for current_task moving from arch/x86 to core code?
Quoted 1 line
But on the whole, I can't claim that it looks noticeable ;*(
Well, then, I guess I don't feel quite so bad about having prioritized this so low for so long. ;-)

One other thing -- may I add your Tested-by to the series?

Thanx, Paul