Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 12 Jul 2022 10:50:28 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH 12/13] locking/qspinlock: separate pv_wait_node from the
 non-paravirt path
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Boqun Feng <boqun.feng@gmail.com>, linux-kernel@vger.kernel.org,
        Waiman Long <longman@redhat.com>,
        Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>
References: <20220704143820.3071004-1-npiggin@gmail.com>
        <20220704143820.3071004-13-npiggin@gmail.com>
        <YsR2IQsnqAOgDxXu@worktop.programming.kicks-ass.net>
In-Reply-To: <YsR2IQsnqAOgDxXu@worktop.programming.kicks-ass.net>
MIME-Version: 1.0
Message-Id: <1657586928.uawdad42k7.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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.
>>=20
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  kernel/locking/qspinlock.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>=20
>> 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(stru=
ct qspinlock *lock, bool parav
>>  		/* Link @node into the waitqueue. */
>>  		WRITE_ONCE(prev->next, node);
>> =20
>> +		/* 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);
>> =20
>=20
> (from patch #6):
>=20
> +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 ac=
tually
> +        * spin-wait for it. We do however rely on our caller to do a
> +        * load-acquire for us.
> +        */
> +}
>=20
>=20

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

Thanks,
Nick