Messages in this thread Patch in this message | | | From | Thomas Gleixner <> | Subject | Re: [patch V9 02/39] rcu: Abstract out rcu_irq_enter_check_tick() from rcu_nmi_enter() | Date | Thu, 21 May 2020 23:25:32 +0200 |
| |
"Paul E. McKenney" <paulmck@kernel.org> writes: > On Thu, May 21, 2020 at 10:05:15PM +0200, Thomas Gleixner wrote: >> +void __rcu_irq_enter_check_tick(void); >> + >> +static __always_inline void rcu_irq_enter_check_tick(void) >> +{ >> + if (context_tracking_enabled()) >> + __rcu_irq_enter_check_tick(); > > I suggest moving the WARN_ON_ONCE(in_nmi()) check here to avoid calling > in_nmi() twice. Because of the READ_ONCE(), the compiler cannot (had > better not!) eliminate the double call.
Makes sense.
>> +void __rcu_irq_enter_check_tick(void) >> +{ >> + struct rcu_data *rdp = this_cpu_ptr(&rcu_data); >> + >> + // Enabling the tick is unsafe in NMI handlers. > > There is an extra space before the "//", probably the one that used to > be after the ";" below. ;-)
This is caused by my fundamental and not suppressible disgust of tail comments. They really disturb the reading flow for me.
if (foo) return; // Because ...
makes my pattern recognition stop because the semicolon is usually the end of the statement. But that's not the only reason.
// Because .... if (foo) return;
makes more sense to me because then the comment is explaining the condition and not the outcome. The outcome is obvious when the condition is well explained.
There are a few exceptions where I adjusted, e.g. in macros:
foo(); \ bar_or_something_else(); \
but only when the trailing backslash is properly aligned.
foo(); \ bar_or_something_else(); \
That stops the parser as well.
I know that this is a pet pieve but I can't help it to adjust it when I have a chance to do so :)
>> + if (WARN_ON_ONCE(in_nmi())) >> + return; >> + >> + RCU_LOCKDEP_WARN(rcu_dynticks_curr_cpu_in_eqs(), >> + "Illegal rcu_irq_enter_check_tick() from extended quiescent state"); > > The instrumentation_begin() has disappeared, presumably because > instrumentation is already enabled in the non-RCU code that directly calls > rcu_irq_enter_check_tick(). (I do see the calls in rcu_nmi_enter() > below.)
Yes. The intention here is to make sure that the caller does not misplace it. So if the call is in a non-instrumentable code path then objtool will complain and the developer will hopefully think twice whether this is the right place to wrap the call with instrumentation_* annotations. I know it's based on hope :)
>> + >> + if (!tick_nohz_full_cpu(rdp->cpu) || >> + !READ_ONCE(rdp->rcu_urgent_qs) || >> + READ_ONCE(rdp->rcu_forced_tick)) { >> + // RCU doesn't need nohz_full help from this CPU, or it is >> + // already getting that help. >> + return; >> + } >> + >> + // We get here only when not in an extended quiescent state and >> + // from interrupts (as opposed to NMIs). 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. >> + // Note that the nohz_full state currently cannot change. >> + 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! >> + WRITE_ONCE(rdp->rcu_forced_tick, true); >> + tick_dep_set_cpu(rdp->cpu, TICK_DEP_BIT_RCU); >> + } >> + raw_spin_unlock_rcu_node(rdp->mynode); >> +} >> #endif /* CONFIG_NO_HZ_FULL */ >> >> /** >> @@ -894,26 +955,7 @@ noinstr void rcu_nmi_enter(void) >> incby = 1; >> } else if (!in_nmi()) { > > This can just be "else" given the in_nmi() check in > __rcu_irq_enter_check_tick(), right? Ah, that check got a > WARN_ON_ONCE(), so never mind! > > I guess that will discourage NMI-handler calls to > rcu_irq_enter_check_tick(). ;-)
Exactly.
> It does mean a double call to in_nmi(), though, so should that > WARN_ON_ONCE(in_nmi()) check go into the rcu_irq_enter_check_tick() > wrapper? Or do modern compilers figure this one out? Given the > READ_ONCE() in preempt_count(), I have to say that I hope not. > So see my comment above on rcu_irq_enter_check_tick().
Moving it to the wrapper is the right thing to do. Will fix.
Thanks,
tglx
| |