RCU: implement rcu_read_[un]lock_preempt()

PATCH In review Tejun Heo 3 participants 9 messages Jul 14, 2008 – Aug 1, 2008 RCU

Share & tools

Tejun Heo

author

2008-07-14 05:59 UTC #0 lore raw

PATCH 1/1 RCU: implement rcu_read_[un]lock_preempt()

With the introduction of preemptible RCU, RCU doesn't gurantee that its critical section runs on the CPU it started to run. As there are cases where non-preemptible RCU critical section makes sense, create new RCU read lock variants which turns of preemption - rcu_read_[un]lock_preempt() which are identical to rcu_read_[un]lock() for classic implementation and have enclosing preempt disable/enable for preemptible RCU.

Signed-off-by Tejun Heo <[email protected]>

---
This will be used by following block layer updates. If this and the block changes get acked, it'll be best to push this through block tree.

Thanks.

 include/linux/rcuclassic.h |    2 ++
 include/linux/rcupdate.h   |   18 ++++++++++++++++++
 include/linux/rcupreempt.h |    6 ++++--
 3 files changed, 24 insertions(+), 2 deletions(-)

include/linux/rcuclassic.h +2 -0

index b3aa05b..08c6153 100644
@@ -136,6 +136,8 @@ extern struct lockdep_map rcu_lock_map;
 		__release(RCU); \
 		preempt_enable(); \
 	} while (0)
+#define __rcu_read_lock_preempt()	__rcu_read_lock()
+#define __rcu_read_unlock_preempt()	__rcu_read_unlock()
 #define __rcu_read_lock_bh() \
 	do { \
 		local_bh_disable(); \

include/linux/rcupdate.h +18 -0

index d42dbec..e0e3486 100644
@@ -112,6 +112,24 @@ struct rcu_head {
 #define rcu_read_unlock() __rcu_read_unlock()
 
 /**
+ * rcu_read_lock_preempt - mark the beginning of non-preemptible RCU
+ *			   critical section
+ *
+ * Identical to rcu_read_lock() but the critical section is guaranteed
+ * to be non-preemptible.  Note that this is identical to
+ * rcu_read_lock() on classic RCU implementation.
+ */
+#define rcu_read_lock_preempt() __rcu_read_lock_preempt()
+
+/**
+ * rcu_read_unlock_preempt - mark the end of of non-preemptible RCU
+ *			     critical section
+ *
+ * See rcu_read_lock_preempt() for more information.
+ */
+#define rcu_read_unlock_preempt() __rcu_read_unlock_preempt()
+
+/**
  * rcu_read_lock_bh - mark the beginning of a softirq-only RCU critical section
  *
  * This is equivalent of rcu_read_lock(), but to be used when updates

include/linux/rcupreempt.h +4 -2

index 8a05c7e..b263ceb 100644
@@ -49,8 +49,10 @@ extern void __rcu_read_unlock(void)	__releases(RCU);
 extern int rcu_pending(int cpu);
 extern int rcu_needs_cpu(int cpu);
 
-#define __rcu_read_lock_bh()	{ rcu_read_lock(); local_bh_disable(); }
-#define __rcu_read_unlock_bh()	{ local_bh_enable(); rcu_read_unlock(); }
+#define __rcu_read_lock_preempt()	{ rcu_read_lock(); preempt_disable(); }
+#define __rcu_read_unlock_preempt()	{ preempt_enable(); rcu_read_unlock(); }
+#define __rcu_read_lock_bh()		{ rcu_read_lock(); local_bh_disable(); }
+#define __rcu_read_unlock_bh()		{ local_bh_enable(); rcu_read_unlock(); }
 
 extern void __synchronize_sched(void);

Signature · 1 line

1.5.4.5

Peter Zijlstra

2008-07-16 06:07 UTC #1 lore raw

Quoted 9 lines · Tejun Heo

With the introduction of preemptible RCU, RCU doesn't gurantee that its critical section runs on the CPU it started to run. As there are cases where non-preemptible RCU critical section makes sense, create new RCU read lock variants which turns of preemption - rcu_read_[un]lock_preempt() which are identical to rcu_read_[un]lock() for classic implementation and have enclosing preempt disable/enable for preemptible RCU.
Signed-off-by Tejun Heo <[email protected]>

Sorry, NAK.

If you need preempt off you need it for other reasons than RCU, so mixing it in the interface doesn't make sense to me.

Quoted 71 lines

---
This will be used by following block layer updates. If this and the block changes get acked, it'll be best to push this through block tree.

Thanks.

 include/linux/rcuclassic.h |    2 ++
 include/linux/rcupdate.h   |   18 ++++++++++++++++++
 include/linux/rcupreempt.h |    6 ++++--
 3 files changed, 24 insertions(+), 2 deletions(-)

include/linux/rcuclassic.h +2 -0

index b3aa05b..08c6153 100644
@@ -136,6 +136,8 @@ extern struct lockdep_map rcu_lock_map;
 		__release(RCU); \
 		preempt_enable(); \
 	} while (0)
+#define __rcu_read_lock_preempt()	__rcu_read_lock()
+#define __rcu_read_unlock_preempt()	__rcu_read_unlock()
 #define __rcu_read_lock_bh() \
 	do { \
 		local_bh_disable(); \

include/linux/rcupdate.h +18 -0

index d42dbec..e0e3486 100644
@@ -112,6 +112,24 @@ struct rcu_head {
 #define rcu_read_unlock() __rcu_read_unlock()
 
 /**
+ * rcu_read_lock_preempt - mark the beginning of non-preemptible RCU
+ *			   critical section
+ *
+ * Identical to rcu_read_lock() but the critical section is guaranteed
+ * to be non-preemptible.  Note that this is identical to
+ * rcu_read_lock() on classic RCU implementation.
+ */
+#define rcu_read_lock_preempt() __rcu_read_lock_preempt()
+
+/**
+ * rcu_read_unlock_preempt - mark the end of of non-preemptible RCU
+ *			     critical section
+ *
+ * See rcu_read_lock_preempt() for more information.
+ */
+#define rcu_read_unlock_preempt() __rcu_read_unlock_preempt()
+
+/**
  * rcu_read_lock_bh - mark the beginning of a softirq-only RCU critical section
  *
  * This is equivalent of rcu_read_lock(), but to be used when updates

include/linux/rcupreempt.h +4 -2

index 8a05c7e..b263ceb 100644
@@ -49,8 +49,10 @@ extern void __rcu_read_unlock(void)	__releases(RCU);
 extern int rcu_pending(int cpu);
 extern int rcu_needs_cpu(int cpu);
 
-#define __rcu_read_lock_bh()	{ rcu_read_lock(); local_bh_disable(); }
-#define __rcu_read_unlock_bh()	{ local_bh_enable(); rcu_read_unlock(); }
+#define __rcu_read_lock_preempt()	{ rcu_read_lock(); preempt_disable(); }
+#define __rcu_read_unlock_preempt()	{ preempt_enable(); rcu_read_unlock(); }
+#define __rcu_read_lock_bh()		{ rcu_read_lock(); local_bh_disable(); }
+#define __rcu_read_unlock_bh()		{ local_bh_enable(); rcu_read_unlock(); }
 
 extern void __synchronize_sched(void);

Tejun Heo

author

2008-07-19 10:23 UTC #2 lore raw

Quoted 9 lines · Peter Zijlstra

Quoted 8 lines · Tejun Heo
With the introduction of preemptible RCU, RCU doesn't gurantee that its critical section runs on the CPU it started to run. As there are cases where non-preemptible RCU critical section makes sense, create new RCU read lock variants which turns of preemption - rcu_read_[un]lock_preempt() which are identical to rcu_read_[un]lock() for classic implementation and have enclosing preempt disable/enable for preemptible RCU.

Quoted 6 lines

Quoted 1 line
Signed-off-by Tejun Heo <[email protected]>
Sorry, NAK.
If you need preempt off you need it for other reasons than RCU, so mixing it in the interface doesn't make sense to me.

Hmmm... the point of the interface is avoiding doing double preemption operations as on common configurations rcu_read_lock() disables preemption. Yes, it's for different purposes but we have two partially overlapping ops and implementing combined / collapsed ops for such cases is acceptable, I think.

Using get_cpu() or separate preempt_disable() wouldn't incur noticeable performance difference as preemption is really cheap to manipulate but both per-cpu and RCU are for performance optimization and I think having combined ops is a good idea.

I wonder what other people think. If it's agreed that having combined ops is a bad idea, I'll convert it to get_cpu().

Thanks.

Signature · 1 line

tejun

Peter Zijlstra

2008-07-28 15:13 UTC #3 lore raw

Quoted 12 lines · Tejun Heo

Quoted 11 lines · Peter Zijlstra
Quoted 9 lines · Tejun Heo
With the introduction of preemptible RCU, RCU doesn't gurantee that its critical section runs on the CPU it started to run. As there are cases where non-preemptible RCU critical section makes sense, create new RCU read lock variants which turns of preemption - rcu_read_[un]lock_preempt() which are identical to rcu_read_[un]lock() for classic implementation and have enclosing preempt disable/enable for preemptible RCU.
Signed-off-by Tejun Heo <[email protected]>

Quoted 8 lines

Quoted 4 lines
Sorry, NAK.
If you need preempt off you need it for other reasons than RCU, so mixing it in the interface doesn't make sense to me.
Hmmm... the point of the interface is avoiding doing double preemption operations as on common configurations rcu_read_lock() disables preemption.

Should be really cheap then, because the cacheline is already hot.

Quoted 3 lines

Yes, it's for different purposes but we have two partially
overlapping ops and implementing combined / collapsed ops for such cases is acceptable, I think.

They only overlap for !PREEMPT_RCU || !PREEMPT_RT

Quoted 4 lines

Using get_cpu() or separate preempt_disable() wouldn't incur noticeable performance difference as preemption is really cheap to manipulate but both per-cpu and RCU are for performance optimization and I think having combined ops is a good idea.

I don't as its a nightmare to sort out on -rt, where get_cpu() can be converted to get_cpu_locked(), and rcu_read_lock() never disables preemption.

If you convert it to use get_cpu() the conversion is easy, if you introduce this collapsed primitive we're up shit creek because it doesn't map.

Nor does it tell us why you need preempt disabled. Making it just as bad as open-coded preempt_disable()s.

Tejun Heo

author

2008-07-29 01:49 UTC #4 lore raw

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines
Quoted 2 lines
If you need preempt off you need it for other reasons than RCU, so mixing it in the interface doesn't make sense to me.
Hmmm... the point of the interface is avoiding doing double preemption operations as on common configurations rcu_read_lock() disables preemption.
Should be really cheap then, because the cacheline is already hot.

Yeah, it is, so it is eventually a peripheral issue.

Quoted 5 lines

Quoted 3 lines
Yes, it's for different purposes but we have two partially
overlapping ops and implementing combined / collapsed ops for such cases is acceptable, I think.
They only overlap for !PREEMPT_RCU || !PREEMPT_RT

That part is pretty large tho.

Quoted 5 lines

Quoted 4 lines
Using get_cpu() or separate preempt_disable() wouldn't incur noticeable performance difference as preemption is really cheap to manipulate but both per-cpu and RCU are for performance optimization and I think having combined ops is a good idea.

Quoted 7 lines

I don't as its a nightmare to sort out on -rt, where get_cpu() can be converted to get_cpu_locked(), and rcu_read_lock() never disables preemption.
If you convert it to use get_cpu() the conversion is easy, if you introduce this collapsed primitive we're up shit creek because it doesn't map.

I don't get it. So, rcu_read_lock(); preempt_disable(); doesn't map for RT?

Quoted 2 lines

Nor does it tell us why you need preempt disabled. Making it just as bad as open-coded preempt_disable()s.

Heh.. the code probably would have used preempt_disable() if it were not for the combined ops, so the objection is about using preempt_disable()?

Signature · 1 line

tejun

Peter Zijlstra

2008-07-29 06:16 UTC #5 lore raw

Quoted 11 lines · Tejun Heo

Quoted 7 lines · Peter Zijlstra
Quoted 5 lines
Quoted 2 lines
If you need preempt off you need it for other reasons than RCU, so mixing it in the interface doesn't make sense to me.
Hmmm... the point of the interface is avoiding doing double preemption operations as on common configurations rcu_read_lock() disables preemption.
Should be really cheap then, because the cacheline is already hot.
Yeah, it is, so it is eventually a peripheral issue.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Yes, it's for different purposes but we have two partially
overlapping ops and implementing combined / collapsed ops for such cases is acceptable, I think.
They only overlap for !PREEMPT_RCU || !PREEMPT_RT
That part is pretty large tho.

But the fact is that PREEMPT_RCU is upstream now..

Quoted 9 lines

Quoted 9 lines
Quoted 4 lines
Using get_cpu() or separate preempt_disable() wouldn't incur noticeable performance difference as preemption is really cheap to manipulate but both per-cpu and RCU are for performance optimization and I think having combined ops is a good idea.
I don't as its a nightmare to sort out on -rt, where get_cpu() can be converted to get_cpu_locked(), and rcu_read_lock() never disables preemption.

Quoted 5 lines

Quoted 3 lines
If you convert it to use get_cpu() the conversion is easy, if you introduce this collapsed primitive we're up shit creek because it doesn't map.
I don't get it. So, rcu_read_lock(); preempt_disable(); doesn't map for RT?

Nope, we'd want something like:

  rcu_read_lock();

  cpu = get_cpu();
  data = percpu_ptr(ptr, cpu);
  fiddle_per_cpu_data();
  put_cpu();

  do_more_rcu_protected_stuff();

  rcu_read_unlock();

This clearly shows the intent - on -rt we can then easily convert the per-cpu stuff to use a lock instead of disabling preemption. Making the whole code sequence preemptible.

If you would have mixed it into the rcu ops, its unclear what code needs protection from what.

Quoted 5 lines

Quoted 2 lines
Nor does it tell us why you need preempt disabled. Making it just as bad as open-coded preempt_disable()s.
Heh.. the code probably would have used preempt_disable() if it were not for the combined ops, so the objection is about using preempt_disable()?

preempt_disable() is just like the BKL, its opaque in that it isn't bound to some data and doesn't tell you what it protects. So getting rid of them afterwards is pain.

Therefore we prefer APIs such as, get_cpu() etc.. that show intent and don;t like stuff you just now proposed of mixing intents. Because when changing the preemption model you might need different solution for different intents.

Tejun Heo

author

2008-07-30 01:18 UTC #6 lore raw

Quoted 4 lines · Peter Zijlstra

Therefore we prefer APIs such as, get_cpu() etc.. that show intent and don;t like stuff you just now proposed of mixing intents. Because when changing the preemption model you might need different solution for different intents.

Alright, will convert to get_cpu(). Thanks for the explanation.

Signature · 1 line

tejun

Paul E. McKenney

2008-08-01 21:16 UTC #7 lore raw

In reply to Peter Zijlstra

Quoted 9 lines · Peter Zijlstra

Quoted 8 lines · Tejun Heo
With the introduction of preemptible RCU, RCU doesn't gurantee that its critical section runs on the CPU it started to run. As there are cases where non-preemptible RCU critical section makes sense, create new RCU read lock variants which turns of preemption - rcu_read_[un]lock_preempt() which are identical to rcu_read_[un]lock() for classic implementation and have enclosing preempt disable/enable for preemptible RCU.

Quoted 6 lines

Quoted 1 line
Signed-off-by Tejun Heo <[email protected]>
Sorry, NAK.
If you need preempt off you need it for other reasons than RCU, so mixing it in the interface doesn't make sense to me.

What Peter said.

For example, you could simply use preempt_disable() and preempt_enable() to guard the read side (or wrapper these as Mathieu suggests in
http://lkml.org/lkml/2008/7/15/605, though Mathieu has not yet convinced me that these wrappers are a good idea), and then use either call_rcu_sched() or synchronize_sched() for the update side.

To summarize:

o Replace your rcu_read_lock_preempt() with preempt_disable().

o Replace your rcu_read_unlock_preempt() with preempt_enable().

o Replace your use of call_rcu() with call_rcu_sched().

o Replace your use of synchronize_rcu() with synchronize_sched().

And then you don't need these new primitives.

However!!! This must be done carefully, as long sections of code with preemption disabled are really bad for realtime response.

So, what are you really trying to do, Tejun?

Thanx, Paul

Quoted 72 lines

Quoted 71 lines

---
This will be used by following block layer updates. If this and the block changes get acked, it'll be best to push this through block tree.

Thanks.

 include/linux/rcuclassic.h |    2 ++
 include/linux/rcupdate.h   |   18 ++++++++++++++++++
 include/linux/rcupreempt.h |    6 ++++--
 3 files changed, 24 insertions(+), 2 deletions(-)

include/linux/rcuclassic.h +2 -0

index b3aa05b..08c6153 100644
@@ -136,6 +136,8 @@ extern struct lockdep_map rcu_lock_map;
 		__release(RCU); \
 		preempt_enable(); \
 	} while (0)
+#define __rcu_read_lock_preempt()	__rcu_read_lock()
+#define __rcu_read_unlock_preempt()	__rcu_read_unlock()
 #define __rcu_read_lock_bh() \
 	do { \
 		local_bh_disable(); \

include/linux/rcupdate.h +18 -0

index d42dbec..e0e3486 100644
@@ -112,6 +112,24 @@ struct rcu_head {
 #define rcu_read_unlock() __rcu_read_unlock()
 
 /**
+ * rcu_read_lock_preempt - mark the beginning of non-preemptible RCU
+ *			   critical section
+ *
+ * Identical to rcu_read_lock() but the critical section is guaranteed
+ * to be non-preemptible.  Note that this is identical to
+ * rcu_read_lock() on classic RCU implementation.
+ */
+#define rcu_read_lock_preempt() __rcu_read_lock_preempt()
+
+/**
+ * rcu_read_unlock_preempt - mark the end of of non-preemptible RCU
+ *			     critical section
+ *
+ * See rcu_read_lock_preempt() for more information.
+ */
+#define rcu_read_unlock_preempt() __rcu_read_unlock_preempt()
+
+/**
  * rcu_read_lock_bh - mark the beginning of a softirq-only RCU critical section
  *
  * This is equivalent of rcu_read_lock(), but to be used when updates

include/linux/rcupreempt.h +4 -2

index 8a05c7e..b263ceb 100644
@@ -49,8 +49,10 @@ extern void __rcu_read_unlock(void)	__releases(RCU);
 extern int rcu_pending(int cpu);
 extern int rcu_needs_cpu(int cpu);
 
-#define __rcu_read_lock_bh()	{ rcu_read_lock(); local_bh_disable(); }
-#define __rcu_read_unlock_bh()	{ local_bh_enable(); rcu_read_unlock(); }
+#define __rcu_read_lock_preempt()	{ rcu_read_lock(); preempt_disable(); }
+#define __rcu_read_unlock_preempt()	{ preempt_enable(); rcu_read_unlock(); }
+#define __rcu_read_lock_bh()		{ rcu_read_lock(); local_bh_disable(); }
+#define __rcu_read_unlock_bh()		{ local_bh_enable(); rcu_read_unlock(); }
 
 extern void __synchronize_sched(void);

Peter Zijlstra

2008-08-01 23:06 UTC #8 lore raw

Quoted 31 lines · Paul E. McKenney

Quoted 9 lines · Peter Zijlstra
Quoted 8 lines · Tejun Heo
With the introduction of preemptible RCU, RCU doesn't gurantee that its critical section runs on the CPU it started to run. As there are cases where non-preemptible RCU critical section makes sense, create new RCU read lock variants which turns of preemption - rcu_read_[un]lock_preempt() which are identical to rcu_read_[un]lock() for classic implementation and have enclosing preempt disable/enable for preemptible RCU.
Quoted 6 lines
Quoted 1 line
Signed-off-by Tejun Heo <[email protected]>
Sorry, NAK.
If you need preempt off you need it for other reasons than RCU, so mixing it in the interface doesn't make sense to me.
What Peter said.
For example, you could simply use preempt_disable() and preempt_enable() to guard the read side (or wrapper these as Mathieu suggests in
http://lkml.org/lkml/2008/7/15/605, though Mathieu has not yet convinced me that these wrappers are a good idea), and then use either call_rcu_sched() or synchronize_sched() for the update side.
To summarize:
o Replace your rcu_read_lock_preempt() with preempt_disable().
o Replace your rcu_read_unlock_preempt() with preempt_enable().

Quoted 8 lines

o Replace your use of call_rcu() with call_rcu_sched().
o Replace your use of synchronize_rcu() with synchronize_sched().
And then you don't need these new primitives.
However!!! This must be done carefully, as long sections of code with preemption disabled are really bad for realtime response.

Right - what I said in the other mail, we should really not use the sched-RCU variant if we can avoid it.