Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 22 Jun 2018 06:36:31 -0700
From:   "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To:     Byungchul Park <byungchul.park@lge.com>
Cc:     Byungchul Park <max.byungchul.park@gmail.com>,
        jiangshanlai@gmail.com, josh@joshtriplett.org,
        Steven Rostedt <rostedt@goodmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        linux-kernel@vger.kernel.org, kernel-team@lge.com,
        Joel Fernandes <joel@joelfernandes.org>, luto@kernel.org
Subject: Re: [RFC 2/2] rcu: Remove ->dynticks_nmi_nesting from struct
 rcu_dynticks
Reply-To: paulmck@linux.vnet.ibm.com
References: <1529484440-20634-1-git-send-email-byungchul.park@lge.com>
 <1529484440-20634-2-git-send-email-byungchul.park@lge.com>
 <20180620145814.GQ3593@linux.vnet.ibm.com>
 <CANrsvRMYc5u3L53nnnGC6p-QcjBTfLPjkJGZyubGWAPauc-W8w@mail.gmail.com>
 <20180620164902.GW3593@linux.vnet.ibm.com>
 <CANrsvRPAWv8LgSvPrGgLiXnjrtK3CmpJgdpNsuu-0LZJTYeYWQ@mail.gmail.com>
 <20180620174037.GZ3593@linux.vnet.ibm.com>
 <20180621063949.GA28024@X58A-UD3R>
 <20180621150407.GE3593@linux.vnet.ibm.com>
 <20180622030032.GB17010@X58A-UD3R>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180622030032.GB17010@X58A-UD3R>
User-Agent: Mutt/1.5.21 (2010-09-15)
Message-Id: <20180622133631.GO3593@linux.vnet.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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