linux-kernel - Re: Simplifying our RCU models

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180306084738.tcs4ggbby77phlbh@gmail.com>
Date:   Tue, 6 Mar 2018 09:47:38 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Tejun Heo <tj@...nel.org>, Jann Horn <jannh@...gle.com>,
        Benjamin LaHaise <bcrl@...ck.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        linux-kernel@...r.kernel.org
Subject: Re: Simplifying our RCU models


* Paul E. McKenney <paulmck@...ux.vnet.ibm.com> wrote:

> > > But if we look at the bigger API picture:
> > >
> > >                           !PREEMPT_RCU  PREEMPT_RCU=y
> > >   rcu_read_lock():        atomic        preemptible
> > >   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?

Yes, that approach sounds very reasonable to me: it is similar to what we do on 
the locking side as well, where we have 'atomic' variants (spinlocks/rwlocks) and 
'sleeping' variants (mutexes, rwsems, etc.).

( This means there will be more automatic coupling between BH and preempt critical
  sections and RCU models not captured via explicit RCU-namespace APIs, but that
  should be OK I think. )

A couple of small side notes:

 - Could we please also clean up the namespace of the synchronization APIs and 
   change them all to an rcu_ prefix, like all the other RCU APIs are? Right now 
   have a mixture like rcu_read_lock() but synchronize_rcu(), while I'd reall love 
   to be able to do:

	git grep '\<rcu_' ...

   ... to see RCU API usage within a particular kernel area. This would also clean
   up some of the internal inconsistencies like having 'struct rcu_synchronize'.

 - If we are cleaning up the write side APIs, could we move over to a _wait 
   nomenclature, i.e. rcu_wait*()?

   I.e. the new RCU namespace would be something like:

     rcu_read_lock                => rcu_read_lock        # unchanged
     rcu_read_unlock              => rcu_read_unlock      # unchanged

     call_rcu                     => rcu_call_rcu
     call_rcu_bh                  => rcu_call_bh
     call_rcu_sched               => rcu_call_sched

     synchronize_rcu              => rcu_wait_
     synchronize_rcu_bh           => rcu_wait_bh
     synchronize_rcu_bh_expedited => rcu_wait_expedited_bh
     synchronize_rcu_expedited    => rcu_wait_expedited
     synchronize_rcu_mult         => rcu_wait_mult
     synchronize_rcu_sched        => rcu_wait_sched
     synchronize_rcu_tasks        => rcu_wait_tasks

     srcu_read_lock               => srcu_read_lock       # unchanged
     srcu_read_unlock             => srcu_read_unlock     # unchanged

     synchronize_srcu             => srcu_wait
     synchronize_srcu_expedited   => srcu_wait_expedited

   Note that due to the prefix approach we gain various new patterns:

       git grep rcu_wait          # matches both rcu and srcu
       git grep rcu_wait          # matches all RCU waiting variants
       git grep wait_expedited    # matches all expedited variants

   ... which all increase the organization of the namespace.

 - While we are at it, the two RCU-state API variants, while rarely used, are
   named in a pretty obscure, disconnected fashion as well. A much better naming 
   would be:

     get_state_synchronize_rcu    => rcu_get_state
     cond_synchronize_rcu         => rcu_wait_state

   ... or so. This would also move them into the new, unified rcu_ prefix 
   namespace.

Note how consistent and hierarchical the new RCU API namespace is:

	<subsystem-prefix>_<verb>[_<qualifier[s]>]

If you agree with the overall concept of this I'd be glad to help out with 
scripting & testing the RCU namespace transition safely in an unintrusive fashion 
once you've done the model unification work, with compatibility defines to not 
create conflicts, churn and pain, etc.

Thanks,

	Ingo