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]

Message-ID: <571def82-92c8-e7e5-74d6-4a7cfc225977@quicinc.com>
Date:   Mon, 13 Dec 2021 14:41:13 +0530
From:   Neeraj Upadhyay <quic_neeraju@...cinc.com>
To:     David Woodhouse <dwmw2@...radead.org>,
        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 12/13/2021 2:27 PM, David Woodhouse wrote:
> 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.
> 	 */
> 

Looks good to me!



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

Thanks; the 4 RCU specific patches in that tree looks good to me.


Thanks
Neeraj

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