Re: Simplifying our RCU models

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Fri, Mar 09, 2018 at 05:48:55PM +0800, Lai Jiangshan wrote:
> On Tue, Mar 6, 2018 at 12:14 AM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Mon, Mar 05, 2018 at 08:33:20AM -0600, Eric W. Biederman wrote:
> >>
> >> Moving this discussion to a public list as discussing how to reduce the
> >> number of rcu variants does not make sense in private.  We should have
> >> an archive of such discussions.
> >>
> >> Ingo Molnar <mingo@kernel.org> writes:
> >>
> >> > * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> >> >
> >> >> > So if people really want that low-cost RCU, and some people really
> >> >> > need the sleepable version, the only one that can _possibly_ be dumped
> >> >> > is the preempt one.
> >> >> >
> >> >> > But I may - again - be confused and/or missing something.
> >> >>
> >> >> I am going to do something very stupid and say that I was instead thinking in
> >> >> terms of getting rid of RCU-bh, thus reminding you of its existence.  ;-)
> >> >>
> >> >> The reason for believing that it is possible to get rid of RCU-bh is the work
> >> >> that has gone into improving the forward progress of RCU grace periods under
> >> >> heavy load and in corner-case workloads.
> >> >>
> >> >
> >> > [...]
> >> >
> >> >> The other reason for RCU-sched is it has the side effect of waiting
> >> >> for all in-flight hardware interrupt handlers, NMI handlers, and
> >> >> preempt-disable regions of code to complete, and last I checked, this side
> >> >> effect is relied on.  In contrast, RCU-preeempt is only guaranteed to wait
> >> >> on regions of code protected by rcu_read_lock() and rcu_read_unlock().
> >> >
> >> > Instead of only trying to fix the documentation (which is never a bad idea but it
> >> > is fighting the symptom in this case), I think the first step should be to
> >> > simplify the RCU read side APIs of RCU from 4 APIs:
> >> >
> >> >     rcu_read_lock()
> >> >     srcu_read_lock()
> >> >     rcu_read_lock_sched()
> >> >     rcu_read_lock_bh()
> >> >
> >> > ... which have ~8 further sub-model variations depending on CONFIG_PREEMPT,
> >> > CONFIG_PREEMPT_RCU - which is really a crazy design!
> >
> > If it is possible to set CONFIG_PREEMPT_RCU differently than CONFIG_PREEMPT,
> > then that is a bug that I need to fix.
> >
> >> > I think we could reduce this to just two APIs with no Kconfig dependencies:
> >> >
> >> >     rcu_read_lock()
> >> >     rcu_read_lock_preempt_disable()
> >> >
> >> > Which would be much, much simpler.
> >
> > No argument on the simpler part, at least from an API perspective.
> >
> >> > This is how we could do it I think:
> >> >
> >> > 1)
> >> >
> >> > Getting rid of the _bh() variant should be reasonably simple and involve a
> >> > treewide replacement of:
> >> >
> >> >     rcu_read_lock_bh()   -> local_bh_disable()
> >> >     rcu_read_unlock_bh() -> local_bh_enable()
> >> >
> >> > Correct?
> >
> > Assuming that I have done enough forward-progress work on grace periods, yes.
> >
> >> > 2)
> >> >
> >> > Further reducing the variants is harder, due to this main asymmetry:
> >> >
> >> >                           !PREEMPT_RCU  PREEMPT_RCU=y
> >> >   rcu_read_lock_sched():  atomic        atomic
> >> >   rcu_read_lock():        atomic        preemptible
> >> >
> >> > ('atomic' here is meant in the scheduler, non-preemptible sense.)
> >> >
> >> > But if we look at the bigger API picture:
> >> >
> >> >                           !PREEMPT_RCU  PREEMPT_RCU=y
> >> >   rcu_read_lock():        atomic        preemptiblep
> >> >   rcu_read_lock_sched():  atomic        atomic
> >> >   srcu_read_lock():       preemptible   preemptible
> >> >
> >> > Then we could maintain full read side API flexibility by making PREEMPT_RCU=y the
> >> > only model, merging it with SRCU and using these main read side APIs:
> >> >
> >> >   rcu_read_lock_preempt_disable((): atomic
> >> >   rcu_read_lock()                   preemptible
> >
> > One issue with merging SRCU into rcu_read_lock() is the general blocking
> > within SRCU readers.  Once merged in, these guys block everyone.  We should
> > focus initially on the non-SRCU variants.
> >
> > On the other hand, Linus's suggestion of merging rcu_read_lock_sched()
> > into rcu_read_lock() just might be feasible.  If that really does pan
> > out, we end up with the following:
> >
> >                                 !PREEMPT        PREEMPT=y
> >         rcu_read_lock():        atomic          preemptible
> >         srcu_read_lock():       preemptible     preemptible
> >
> > In this model, rcu_read_lock_sched() maps to preempt_disable() and (as
> > you say above) rcu_read_lock_bh() maps to local_bh_disable().  The way
> > this works is that in PREEMPT=y kernels, synchronize_rcu() waits not
> > only for RCU read-side critical sections, but also for regions of code
> > with preemption disabled.  The main caveat seems to be that there be an
> > assumed point of preemptibility between each interrupt and each softirq
> > handler, which should be OK.
> >
> > There will be some adjustments required for lockdep-RCU, but that should
> > be reasonably straightforward.
> >
> > Seem reasonable?
> 
> It's good. I hope there is only one global(non-srcu) rcu variant.

Well, there will still be both SRCU and RCU-tasks, but reducing by
two should at least help.

> It does have the trade-off, the grace period will be extended a little
> in some cases,
> so will the call_rcu()/synchronze_rcu(). But it simplifies the coding a lot.

True, and the extended grace periods did bother me at first.  But then
I realized that it was no worse than RCU-sched, so it should be OK.

And thank you for looking this over!

							Thanx, Paul

> >> > It's a _really_ simple and straightforward RCU model, with very obvious semantics
> >> > all around:
> >> >
> >> > - Note how the 'atomic' (non-preempt) variant uses the well-known
> >> >   preempt_disable() name as a postfix to signal its main property. (It's also a
> >> >   bit of a mouthful, which should discourage over-use.)
> >
> > My thought is to eliminate the atomic variant entirely.  If you want
> > to disable preemption, interrupts, or whatever, you simply do so.
> > It might turn out that there are documentation benefits to having a
> > separate rcu_read_lock_preempt_disable() that maps to preempt_disable()
> > with lockdep semantics, and if so, that can be provided trivially.
> >
> >> > - The read side APIs are really as straightforward as possible: there's no SRCU
> >> >   distinction on the read side, no _bh() distinction and no _sched() distinction.
> >> >   (On -rt all of these would turn into preemptible sections,
> >> >   obviously.)
> >
> > Agreed, and both models accomplish that.
> >
> >> And it looses the one advantage of srcu_read_lock.  That you don't have
> >> to wait for the entire world.  If you actually allow sleeping that is an
> >> important distinction to have.  Or are you proposing that we add the
> >> equivalent of init_srcu_struct to all of the rcu users?
> >
> > I am instead proposing folding rcu_read_lock_bh() and rcu_read_lock_sched()
> > into rcu_read_lock(), and leaving srcu_read_lock() separate.
> >
> >> That rcu_read_lock would need to take an argument about which rcu region
> >> we are talking about.
> >
> > From what I can see, it would be far better to leave SRCU separate.  As you
> > say, it really does have very different semantics.
> >
> >> > rcu_read_lock_preempt_disable() would essentially be all the current
> >> > rcu_read_lock_sched() users (where the _sched() postfix was a confusing misnomer
> >> > anyway).
> >
> > I agree that rcu_read_lock_preempt_disable() is a better name.
> > We might not need it at all, though.  There are only about 20 uses of
> > rcu_read_lock_sched() in v4.15.  ;-)
> >
> >> > Wrt. merging SRCU and RCU: this can be done by making PREEMPT_RCU=y the one and
> >> > only main RCU model and converting all SRCU users to main RCU. This is relatively
> >> > straightforward to perform, as there are only ~170 SRCU critical sections, versus
> >> > the 3000+ main RCU critical sections ...
> >>
> >> It really sounds like you are talking about adding a requirement that
> >> everyone update their rcu_read_lock() calls with information about which
> >> region you are talking about.  That seems like quite a bit of work.
> >
> > Agreed, merging RCU, RCU-bh, and RCU-sched seems much more straightforward
> > to me from the viewpoint of both usage and implementation.
> >
> >> Doing something implicit when PREEMPT_RCU=y and converting
> >> "rcu_read_lock()" to "srcu_read_lock(&kernel_srcu_region)" only in that
> >> case I can see.
> >>
> >> Except in very specific circustances I don't think I ever want to run a
> >> kernel with PREEMPT_RCU the default.  All of that real time stuff trades
> >> off predictability with performance.  Having lost enough performance to
> >> spectre and meltdown I don't think it makes sense for us all to start
> >> runing predictable^H^H^H^H^H^H^H^H^H^H^H time kernels now.
> >
> > Yes, in PREEMPT=n kernels RCU would act exactly as it does today.
> >
> >> > AFAICS this should be a possible read side design that keeps correctness, without
> >> > considering grace period length patterns, i.e. without considering GC latency and
> >> > scalability aspects.
> >> >
> >> > Before we get into ways to solve the latency and scalability aspects of such a
> >> > simplified RCU model, do you agree with this analysis so far, or have I missed
> >> > something important wrt. correctness?
> >>
> >> RCU region specification.  If we routinely allow preemption of rcu
> >> critical sections for any length of time I can't imagine we will want to
> >> wait for every possible preempted rcu critical section.
> >>
> >> Of course I could see the merge working the other way.  Adding the
> >> debugging we need to find rcu critical secions that are held to long and
> >> shrinking them so we don't need PREEMPT_RCU at all.
> >
> > Again, from what I can see, merging rcu_read_lock(), rcu_read_lock_sched(),
> > and rcu_read_lock_bh() together should get us to a much better place.
> >
> > Make sense, or am I missing something?
> >
> >                                                         Thanx, Paul
> >
>