[PATCH RFC 0/7] rcu: v2 Inlinable preemptible rcu_read_lock() and rcu_read_unlock()

* [PATCH RFC 0/7] rcu: v2 Inlinable preemptible rcu_read_lock() and rcu_read_unlock()
@ 2012-04-14 16:19 Paul E. McKenney
  2012-04-14 16:20 ` [PATCH RFC 1/7] rcu: Move PREEMPT_RCU preemption to switch_to() invocation Paul E. McKenney
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Paul E. McKenney @ 2012-04-14 16:19 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, patches, torvalds

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(-)

^ permalink raw reply	[flat|nested] 15+ messages in thread