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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87r1xhd02c.fsf@nanos.tec.linutronix.de>
Date:   Tue, 24 Mar 2020 20:28:43 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     paulmck@...nel.org
Cc:     LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        "Joel Fernandes \(Google\)" <joel@...lfernandes.org>,
        "Steven Rostedt \(VMware\)" <rostedt@...dmis.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Brian Gerst <brgerst@...il.com>,
        Juergen Gross <jgross@...e.com>,
        Alexandre Chartre <alexandre.chartre@...cle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Tom Lendacky <thomas.lendacky@....com>,
        Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org
Subject: Re: [RESEND][patch V3 17/23] rcu/tree: Mark the idle relevant functions noinstr

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