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: <20200324195830.GN19865@paulmck-ThinkPad-P72>
Date:   Tue, 24 Mar 2020 12:58:30 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
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

On Tue, Mar 24, 2020 at 08:28:43PM +0100, Thomas Gleixner wrote:
> "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.

Thank you!

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

One of my parents like this.  https://en.wikipedia.org/wiki/Tab_(drink)

							Thanx, Paul

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