"Paul E. McKenney" <paul...@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
