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
