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

[Peter Zijlstra <peterz@infradead.org>]

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.