Re: [RESEND][patch V3 17/23] rcu/tree: Mark the idle relevant functions noinstr

[Thomas Gleixner]

"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.

>>  	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 :)

>>   * 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