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

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

[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 <tj@kernel.org>
---
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(-)

diff --git a/include/linux/rcuclassic.h b/include/linux/rcuclassic.h
index b3aa05b..08c6153 100644
--- a/include/linux/rcuclassic.h
+++ b/include/linux/rcuclassic.h
@@ -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(); \
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index d42dbec..e0e3486 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -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
diff --git a/include/linux/rcupreempt.h b/include/linux/rcupreempt.h
index 8a05c7e..b263ceb 100644
--- a/include/linux/rcupreempt.h
+++ b/include/linux/rcupreempt.h
@@ -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);
 
-- 
1.5.4.5

Re: [PATCH] RCU: implement rcu_read_[un]lock_preempt()

[Peter Zijlstra]

On Mon, 2008-07-14 at 14:57 +0900, Tejun Heo wrote:
> 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 <tj@kernel.org>

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.

> ---
> 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(-)
> 
> diff --git a/include/linux/rcuclassic.h b/include/linux/rcuclassic.h
> index b3aa05b..08c6153 100644
> --- a/include/linux/rcuclassic.h
> +++ b/include/linux/rcuclassic.h
> @@ -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(); \
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index d42dbec..e0e3486 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -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
> diff --git a/include/linux/rcupreempt.h b/include/linux/rcupreempt.h
> index 8a05c7e..b263ceb 100644
> --- a/include/linux/rcupreempt.h
> +++ b/include/linux/rcupreempt.h
> @@ -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);
>  

Re: [PATCH] RCU: implement rcu_read_[un]lock_preempt()

[Tejun Heo]

Peter Zijlstra wrote:
> On Mon, 2008-07-14 at 14:57 +0900, Tejun Heo wrote:
>> 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 <tj@kernel.org>
> 
> 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.

-- 
tejun

Re: [PATCH] RCU: implement rcu_read_[un]lock_preempt()

[Peter Zijlstra]

On Wed, 2008-07-16 at 15:43 +0900, Tejun Heo wrote:
> Peter Zijlstra wrote:
> > On Mon, 2008-07-14 at 14:57 +0900, Tejun Heo wrote:
> >> 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 <tj@kernel.org>
> > 
> > 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.

>   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

> 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.

Re: [PATCH] RCU: implement rcu_read_[un]lock_preempt()

[Tejun Heo]

Peter Zijlstra wrote:
>>> 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.

>>   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.

>> 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.

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

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

-- 
tejun

Re: [PATCH] RCU: implement rcu_read_[un]lock_preempt()

[Peter Zijlstra]

On Tue, 2008-07-29 at 10:47 +0900, Tejun Heo wrote:
> Peter Zijlstra wrote:
> >>> 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.
> 
> >>   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..

> >> 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.
> 
> 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.

> > 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.

Re: [PATCH] RCU: implement rcu_read_[un]lock_preempt()

[Tejun Heo]

Peter Zijlstra wrote:
> 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.

-- 
tejun

Re: [PATCH] RCU: implement rcu_read_[un]lock_preempt()

[Paul E. McKenney]

On Wed, Jul 16, 2008 at 08:07:49AM +0200, Peter Zijlstra wrote:
> On Mon, 2008-07-14 at 14:57 +0900, Tejun Heo wrote:
> > 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 <tj@kernel.org>
> 
> 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

> > ---
> > 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(-)
> > 
> > diff --git a/include/linux/rcuclassic.h b/include/linux/rcuclassic.h
> > index b3aa05b..08c6153 100644
> > --- a/include/linux/rcuclassic.h
> > +++ b/include/linux/rcuclassic.h
> > @@ -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(); \
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index d42dbec..e0e3486 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -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
> > diff --git a/include/linux/rcupreempt.h b/include/linux/rcupreempt.h
> > index 8a05c7e..b263ceb 100644
> > --- a/include/linux/rcupreempt.h
> > +++ b/include/linux/rcupreempt.h
> > @@ -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);
> >  
> 

Re: [PATCH] RCU: implement rcu_read_[un]lock_preempt()

[Peter Zijlstra]

On Fri, 2008-08-01 at 14:10 -0700, Paul E. McKenney wrote:
> On Wed, Jul 16, 2008 at 08:07:49AM +0200, Peter Zijlstra wrote:
> > On Mon, 2008-07-14 at 14:57 +0900, Tejun Heo wrote:
> > > 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 <tj@kernel.org>
> > 
> > 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.

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