linux-kernel - Re: [PATCH v1.1 02/11] rcu: Kill rnp->ofl_seq and use only rcu_state.ofl

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]

Date:   Mon, 13 Dec 2021 08:57:16 +0000
From:   David Woodhouse <dwmw2@...radead.org>
To:     Neeraj Upadhyay <quic_neeraju@...cinc.com>,
        Thomas Gleixner <tglx@...utronix.de>
Cc:     Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        rcu@...r.kernel.org, mimoja@...oja.de, hewenliang4@...wei.com,
        hushiyuan@...wei.com, luolongjun@...wei.com, hejingxian@...wei.com
Subject: Re: [PATCH v1.1 02/11] rcu: Kill rnp->ofl_seq and use only
 rcu_state.ofl_lock for exclusion

On Fri, 2021-12-10 at 09:56 +0530, Neeraj Upadhyay wrote:
> > -	if (rdp->grpmask & rcu_rnp_online_cpus(rnp) || READ_ONCE(rnp->ofl_seq) & 0x1)
> > +	/*
> > +	 * Strictly, we care here about the case where the current CPU is
> > +	 * in rcu_cpu_starting() and thus has an excuse for rdp->grpmask
> > +	 * not being up to date. So arch_spin_is_locked() might have a
> 
> Minor:
> 
> Is this comment right - "thus has an excuse for rdp->grpmask not being 
> up to date"; shouldn't it be "thus has an excuse for rnp->qsmaskinitnext 
> not being up to date"?
> 
> Also, arch_spin_is_locked() also handles the rcu_report_dead() case,
> where raw_spin_unlock_irqrestore_rcu_node() can have a rcu_read_lock 
> from lockdep path with CPU bits already cleared from rnp->qsmaskinitnext?

Good point; thanks. How's this:

	/*
	 * Strictly, we care here about the case where the current CPU is in
	 * rcu_cpu_starting() or rcu_report_dead() and thus has an excuse for
	 * rdp->qsmaskinitnext not being up to date. So arch_spin_is_locked()
	 * might have a false positive if it's held by some *other* CPU, but
	 * that's OK because that just means a false *negative* on the
	 * warning.
	 */

> >  	if (WARN_ON_ONCE(rnp->qsmask & mask)) { /* RCU waiting on incoming CPU? */
> > +		/* rcu_report_qs_rnp() *really* wants some flags to restore */
> > +		unsigned long flags2;
> 
> Minor: checkpatch flags it "Missing a blank line after declarations"

Ack. Also fixed and pushed out to my parallel-5.16 branch at 
https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-5.16

This commit is probably the only one that's strictly needed for that
parallel bringup, but for now I've kept my rcu boost thread mutex
patch, and added your two patches (with minor whitespace fixes). I
think the best option is to let Paul handle them all.

We'll do the final step of actually *enabling* the parallel bringup on
any given architecture only after the various fixes have made their way
in and we've done a proper review of the remaining code paths.



Download attachment "smime.p7s" of type "application/pkcs7-signature" (5174 bytes)