Re: [PATCH tip/core/rcu 07/13] rcu: Move preemption disabling out of __srcu_read_lock()

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

On Tue, Oct 06, 2015 at 10:18:39AM -0700, Josh Triplett wrote:
> On Tue, Oct 06, 2015 at 09:13:42AM -0700, Paul E. McKenney wrote:
> > Currently, __srcu_read_lock() cannot be invoked from restricted
> > environments because it contains calls to preempt_disable() and
> > preempt_enable(), both of which can invoke lockdep, which is a bad
> > idea in some restricted execution modes.  This commit therefore moves
> > the preempt_disable() and preempt_enable() from __srcu_read_lock()
> > to srcu_read_lock().  It also inserts the preempt_disable() and
> > preempt_enable() around the call to __srcu_read_lock() in do_exit().
> 
> What restricted environments do you intend to invoke
> __srcu_read_lock from?
> 
> This change seems fine, but I don't see any change in this patch series
> that needs this, hence my curiosity.

Someone asked me for it, and now I cannot find it.  :-(

Something to the effect of when running unmapped during exception entry
or something like that.  I guess one way to find out would be to remove
the commit and see who complained, but on the other hand, it arguably
makes more sense to have only the bare mechanism is __srcu_read_lock()
and put the environmental protection into srcu_read_lock().

							Thanx, Paul

> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >  include/linux/srcu.h | 5 ++++-
> >  kernel/exit.c        | 2 ++
> >  kernel/rcu/srcu.c    | 2 --
> >  3 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > index bdeb4567b71e..f5f80c5643ac 100644
> > --- a/include/linux/srcu.h
> > +++ b/include/linux/srcu.h
> > @@ -215,8 +215,11 @@ static inline int srcu_read_lock_held(struct srcu_struct *sp)
> >   */
> >  static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
> >  {
> > -	int retval = __srcu_read_lock(sp);
> > +	int retval;
> >  
> > +	preempt_disable();
> > +	retval = __srcu_read_lock(sp);
> > +	preempt_enable();
> >  	rcu_lock_acquire(&(sp)->dep_map);
> >  	return retval;
> >  }
> > diff --git a/kernel/exit.c b/kernel/exit.c
> > index ea95ee1b5ef7..0e93b63bbc59 100644
> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -761,7 +761,9 @@ void do_exit(long code)
> >  	 */
> >  	flush_ptrace_hw_breakpoint(tsk);
> >  
> > +	TASKS_RCU(preempt_disable());
> >  	TASKS_RCU(tasks_rcu_i = __srcu_read_lock(&tasks_rcu_exit_srcu));
> > +	TASKS_RCU(preempt_enable());
> >  	exit_notify(tsk, group_dead);
> >  	proc_exit_connector(tsk);
> >  #ifdef CONFIG_NUMA
> > diff --git a/kernel/rcu/srcu.c b/kernel/rcu/srcu.c
> > index 9e6122540d28..a63a1ea5a41b 100644
> > --- a/kernel/rcu/srcu.c
> > +++ b/kernel/rcu/srcu.c
> > @@ -298,11 +298,9 @@ int __srcu_read_lock(struct srcu_struct *sp)
> >  	int idx;
> >  
> >  	idx = READ_ONCE(sp->completed) & 0x1;
> > -	preempt_disable();
> >  	__this_cpu_inc(sp->per_cpu_ref->c[idx]);
> >  	smp_mb(); /* B */  /* Avoid leaking the critical section. */
> >  	__this_cpu_inc(sp->per_cpu_ref->seq[idx]);
> > -	preempt_enable();
> >  	return idx;
> >  }
> >  EXPORT_SYMBOL_GPL(__srcu_read_lock);
> > -- 
> > 2.5.2
> > 
>