Re: [RESEND][patch V3 17/23] rcu/tree: Mark the idle relevant functions noinstr

[Paul E. McKenney]

On Tue, Mar 24, 2020 at 08:28:43PM +0100, Thomas Gleixner wrote:
> "Paul E. McKenney" <paulmck@xxxxxxxxxx> writes:
> > On Fri, Mar 20, 2020 at 07:00:13PM +0100, Thomas Gleixner wrote:
> 
> >> -void rcu_user_enter(void)
> >> +noinstr void rcu_user_enter(void)
> >>  {
> >>  	lockdep_assert_irqs_disabled();
> >
> > Just out of curiosity -- this means that lockdep_assert_irqs_disabled()
> > must be noinstr, correct?
> 
> Yes. noinstr functions can call other noinstr functions safely. If there
> is a instr_begin() then anything can be called up to the corresponding
> instr_end(). After that the noinstr rule applies again.

Thank you!

> >>  	if (rdp->dynticks_nmi_nesting != 1) {
> >> +		instr_begin();
> >>  		trace_rcu_dyntick(TPS("--="), rdp->dynticks_nmi_nesting, rdp->dynticks_nmi_nesting - 2,
> >>  				  atomic_read(&rdp->dynticks));
> >>  		WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
> >>  			   rdp->dynticks_nmi_nesting - 2);
> >> +		instr_end();
> >>  		return;
> >>  	}
> >>  
> >> +		instr_begin();
> >
> > Indentation?
> 
> Is obviously wrong. You found it so please keep the extra TAB for times
> when you need a spare one :)

One of my parents like this.  https://en.wikipedia.org/wiki/Tab_(drink)

							Thanx, Paul

> >>   * If you add or remove a call to rcu_user_exit(), be sure to test with
> >>   * CONFIG_RCU_EQS_DEBUG=y.
> >>   */
> >> -void rcu_user_exit(void)
> >> +void noinstr rcu_user_exit(void)
> >>  {
> >>  	rcu_eqs_exit(1);
> >>  }
> >> @@ -830,27 +833,33 @@ static __always_inline void rcu_nmi_ente
> >>  			rcu_cleanup_after_idle();
> >>  
> >>  		incby = 1;
> >> -	} else if (irq && tick_nohz_full_cpu(rdp->cpu) &&
> >> -		   rdp->dynticks_nmi_nesting == DYNTICK_IRQ_NONIDLE &&
> >> -		   READ_ONCE(rdp->rcu_urgent_qs) && !rdp->rcu_forced_tick) {
> >> +	} else if (irq) {
> >>  		// We get here only if we had already exited the extended
> >>  		// quiescent state and this was an interrupt (not an NMI).
> >>  		// Therefore, (1) RCU is already watching and (2) The fact
> >>  		// that we are in an interrupt handler and that the rcu_node
> >>  		// lock is an irq-disabled lock prevents self-deadlock.
> >>  		// So we can safely recheck under the lock.
> >
> > The above comment is a bit misleading in this location.
> 
> True
> 
> >> -		raw_spin_lock_rcu_node(rdp->mynode);
> >> -		if (rdp->rcu_urgent_qs && !rdp->rcu_forced_tick) {
> >> -			// A nohz_full CPU is in the kernel and RCU
> >> -			// needs a quiescent state.  Turn on the tick!
> >> -			rdp->rcu_forced_tick = true;
> >> -			tick_dep_set_cpu(rdp->cpu, TICK_DEP_BIT_RCU);
> >> +		instr_begin();
> >> +		if (tick_nohz_full_cpu(rdp->cpu) &&
> >> +		    rdp->dynticks_nmi_nesting == DYNTICK_IRQ_NONIDLE &&
> >> +		    READ_ONCE(rdp->rcu_urgent_qs) && !rdp->rcu_forced_tick) {
> >
> > So how about like this?
> >
> > 			// We get here only if we had already exited
> > 			// the extended quiescent state and this was an
> > 			// interrupt (not an NMI).  Therefore, (1) RCU is
> > 			// already watching and (2) The fact that we are in
> > 			// an interrupt handler and that the rcu_node lock
> > 			// is an irq-disabled lock prevents self-deadlock.
> > 			// So we can safely recheck under the lock.
> 
> Yup
> 
> Thanks,
> 
>         tglx