Re: [patch V9 02/39] rcu: Abstract out rcu_irq_enter_check_tick() from rcu_nmi_enter()

[Thomas Gleixner]

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