| 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
| ||
|
Message-Id: <20180622133631.GO3593@linux.vnet.ibm.com>
Date: Fri, 22 Jun 2018 06:36:31 -0700
From: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To: Byungchul Park <byungchul.park@....com>
Cc: Byungchul Park <max.byungchul.park@...il.com>,
jiangshanlai@...il.com, josh@...htriplett.org,
Steven Rostedt <rostedt@...dmis.org>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
linux-kernel@...r.kernel.org, kernel-team@....com,
Joel Fernandes <joel@...lfernandes.org>, luto@...nel.org
Subject: Re: [RFC 2/2] rcu: Remove ->dynticks_nmi_nesting from struct
rcu_dynticks
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
Powered by blists - more mailing lists