linux-kernel - Re: [PATCH 12/13] locking/qspinlock: separate pv_wait

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: <1657586928.uawdad42k7.astroid@bobo.none>
Date:   Tue, 12 Jul 2022 10:50:28 +1000
From:   Nicholas Piggin <npiggin@...il.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Boqun Feng <boqun.feng@...il.com>, linux-kernel@...r.kernel.org,
        Waiman Long <longman@...hat.com>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>
Subject: Re: [PATCH 12/13] locking/qspinlock: separate pv_wait_node from the
 non-paravirt path

Excerpts from Peter Zijlstra's message of July 6, 2022 3:34 am:
> On Tue, Jul 05, 2022 at 12:38:19AM +1000, Nicholas Piggin wrote:
>> pv_wait_node waits until node->locked is non-zero, no need for the
>> pv case to wait again by also executing the !pv code path.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@...il.com>
>> ---
>>  kernel/locking/qspinlock.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
>> index 9db168753124..19e2f286be0a 100644
>> --- a/kernel/locking/qspinlock.c
>> +++ b/kernel/locking/qspinlock.c
>> @@ -862,10 +862,11 @@ static inline void queued_spin_lock_mcs_queue(struct qspinlock *lock, bool parav
>>  		/* Link @node into the waitqueue. */
>>  		WRITE_ONCE(prev->next, node);
>>  
>> +		/* Wait for mcs node lock to be released */
>>  		if (paravirt)
>>  			pv_wait_node(node, prev);
>> -		/* Wait for mcs node lock to be released */
>> -		smp_cond_load_acquire(&node->locked, VAL);
>> +		else
>> +			smp_cond_load_acquire(&node->locked, VAL);
>>  
> 
> (from patch #6):
> 
> +static void pv_wait_node(struct qnode *node, struct qnode *prev)
> +{
> +       int loop;
> +       bool wait_early;
> +
> 	...
> +
> +       /*
> +        * By now our node->locked should be 1 and our caller will not actually
> +        * spin-wait for it. We do however rely on our caller to do a
> +        * load-acquire for us.
> +        */
> +}
> 
> 

Oh good catch, thanks so much that's a dumb bug. I'll add a 
smp_load_acquire at the end of pv_wait_node where that comment
is.

Thanks,
Nick