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. 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
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!
> > {
> > 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>
--
Thanks,
Byungchul
