Re: [RFC 2/2] rcu: Remove ->dynticks_nmi_nesting from struct rcu_dynticks

[Paul E. McKenney]

On Fri, Jun 22, 2018 at 12:00:32PM +0900, Byungchul Park wrote:
> On Thu, Jun 21, 2018 at 08:04:07AM -0700, Paul E. McKenney wrote:
> 
> > Nothing quite like concurrent programming to help one see one's own
> > mistakes.  ;-)
> 
> Haha.
> 
> > Your reasoning has merit, but the nice thing about keeping "nmi" is
> > that it helps casual readers see that NMIs must be handled.  If we
> > rename this to "irq", we lose that hint and probably leave some
> > readers wondering why the strange increment-by-2 code is there.
> > So let's please keep the current names.
> 
> Got it. I will.
> 
> > >  /**
> > > - * rcu_nmi_exit - inform RCU of exit from NMI context
> > > + * rcu_irq_exit_common - inform RCU of exit from interrupt context
> > >   *
> > > - * If we are returning from the outermost NMI handler that interrupted an
> > > - * RCU-idle period, update rdtp->dynticks and rdtp->dynticks_nmi_nesting
> > > - * to let the RCU grace-period handling know that the CPU is back to
> > > - * being RCU-idle.
> > > + * If we are returning from the outermost interrupt handler that
> > > + * interrupted an RCU-idle period, update rdtp->dynticks and
> > > + * rdtp->dynticks_irq_nesting to let the RCU grace-period handling
> > > + * know that the CPU is back to being RCU-idle.
> > >   *
> > > - * If you add or remove a call to rcu_nmi_exit(), be sure to test
> > > - * with CONFIG_RCU_EQS_DEBUG=y.
> > > + * If you add or remove a call to rcu_irq_exit_common(), be sure to
> > > + * test with CONFIG_RCU_EQS_DEBUG=y.
> > >   */
> > > -void rcu_nmi_exit(void)
> > > +static __always_inline void rcu_irq_exit_common(bool nmi)
> > 
> > However, I suggest making this function's parameter "irq" because ...
> 
> I will.
> 
> > Does the generated code really get rid of the conditional branches?
> > I would hope that it wouild, but it is always good to check.  This
> > should be easy to find in the assembly-language output because of the
> > calls to rcu_prepare_for_idle() and rcu_dynticks_task_enter().
> 
> Good! It works as we expect, I did it only with x86_64 tho.

I suspect that it would be similar for most other architectures running
the same compiler version.  Might be worth firing up a cross-compiler
to check one or two more, though.

>                                                             Let me show
> you the part we are interested in. The rest are almost same.
> 
> <rcu_nmi_exit>:
> 	5b                   	pop    %rbx
> 	5d                   	pop    %rbp
> 	41 5c                	pop    %r12
> 	41 5d                	pop    %r13
> 	41 5e                	pop    %r14
> 	41 5f                	pop    %r15
> 	e9 0f 75 ff ff       	jmpq   ffffffff810bb440 <rcu_dynticks_eqs_enter>
> 
> <rcu_irq_exit>:
> 	e8 e6 e5 ff ff       	callq  ffffffff810c26a0 <rcu_prepare_for_idle>
> 	e8 81 73 ff ff       	callq  ffffffff810bb440 <rcu_dynticks_eqs_enter>
> 	e8 ec 3a 2b 00       	callq  ffffffff81377bb0 <debug_smp_processor_id>
> 	65 48 8b 14 25 00 4d 	mov    %gs:0x14d00,%rdx
> 	01 00 
> 	89 82 94 03 00 00    	mov    %eax,0x394(%rdx)
> 	5b                   	pop    %rbx
> 	5d                   	pop    %rbp
> 	41 5c                	pop    %r12
> 	41 5d                	pop    %r13
> 	41 5e                	pop    %r14
> 	41 5f                	pop    %r15
> 	c3                   	retq

This is a summary view focusing on the function calls, correct?
(I am guessing this based on your "the part we are interested in".)

> Even though they return in a little bit different way, anyway I can see
> all the branchs we are interested in were removed by compiler!

Yes, very nice!

The reason for the difference is that the compiler applied tail
recursion to rcu_nmi_exit()'s call to rcu_dynticks_eqs_enter(), and
inlined rcu_irq_exit()'s call to rcu_dynticks_task_enter().

> > >  {
> > >  	struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
> > >  	long incby = 2;
> > > 
> > >  	/* Complain about underflow. */
> > > -	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 0);
> > > +	WARN_ON_ONCE(rdtp->dynticks_irq_nesting < 0);
> > > 
> > >  	/*
> > >  	 * If idle from RCU viewpoint, atomically increment ->dynticks
> > > -	 * to mark non-idle and increment ->dynticks_nmi_nesting by one.
> > > -	 * Otherwise, increment ->dynticks_nmi_nesting by two.  This means
> > > -	 * if ->dynticks_nmi_nesting is equal to one, we are guaranteed
> > > +	 * to mark non-idle and increment ->dynticks_irq_nesting by one.
> > > +	 * Otherwise, increment ->dynticks_irq_nesting by two.  This means
> > > +	 * if ->dynticks_irq_nesting is equal to one, we are guaranteed
> > >  	 * to be in the outermost NMI handler that interrupted an RCU-idle
> > >  	 * period (observation due to Andy Lutomirski).
> > >  	 */
> > >  	if (rcu_dynticks_curr_cpu_in_eqs()) {
> > > +
> > > +		if (!nmi)
> > > +			rcu_dynticks_task_exit();
> > > +
> > >  		rcu_dynticks_eqs_exit();
> > > +
> > > +		if (!nmi)
> > 
> > ... and checking for branches here.
> 
> Also good! The following is the only different part.
> 
> <rcu_nmi_enter>:
> 	e8 dc 81 ff ff       	callq  ffffffff810bc450 <rcu_dynticks_eqs_exit>
> 
> <rcu_irq_enter>:
> 	65 48 8b 04 25 00 4d 	mov    %gs:0x14d00,%rax
> 	01 00 
> 	c7 80 94 03 00 00 ff 	movl   $0xffffffff,0x394(%rax)
> 	ff ff ff 
> 	e8 b9 80 ff ff       	callq  ffffffff810bc450 <rcu_dynticks_eqs_exit>
> 	e8 d4 b9 ff ff       	callq  ffffffff810bfd70 <rcu_cleanup_after_idle>

Also looks good, so please do send the patches!

							Thanx, Paul