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:06:52 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH 02/13] locking/qspinlock: inline mcs_spinlock functions
 into qspinlock
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-3-npiggin@gmail.com>
        <YsRtaOnO9vlxJvLi@worktop.programming.kicks-ass.net>
In-Reply-To: <YsRtaOnO9vlxJvLi@worktop.programming.kicks-ass.net>
MIME-Version: 1.0
Message-Id: <1657584250.pjnraozr5g.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 2:57 am:
> On Tue, Jul 05, 2022 at 12:38:09AM +1000, Nicholas Piggin wrote:
>> qspinlock uses mcs_spinlock for the struct type (.next, .locked, and the
>> misplaced .count), and arch_mcs_spin_{un}lock_contended(). These can be
>> trivially inlined into qspinlock, and the unused mcs_spinlock code
>> removed.
>>=20
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>=20
>>  arch/arm/include/asm/mcs_spinlock.h |  24 ------
>=20
>> --- a/arch/arm/include/asm/mcs_spinlock.h
>> +++ /dev/null
>> @@ -1,24 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0 */
>> -#ifndef __ASM_MCS_LOCK_H
>> -#define __ASM_MCS_LOCK_H
>> -
>> -#ifdef CONFIG_SMP
>> -#include <asm/spinlock.h>
>> -
>> -/* MCS spin-locking. */
>> -#define arch_mcs_spin_lock_contended(lock)				\
>> -do {									\
>> -	/* Ensure prior stores are observed before we enter wfe. */	\
>> -	smp_mb();							\
>> -	while (!(smp_load_acquire(lock)))				\
>> -		wfe();							\
>> -} while (0)								\
>> -
>> -#define arch_mcs_spin_unlock_contended(lock)				\
>> -do {									\
>> -	smp_store_release(lock, 1);					\
>> -	dsb_sev();							\
>> -} while (0)
>> -
>> -#endif	/* CONFIG_SMP */
>> -#endif	/* __ASM_MCS_LOCK_H */
>=20
>> @@ -475,7 +476,8 @@ void queued_spin_lock_slowpath(struct qspinlock *loc=
k, u32 val)
>>  		WRITE_ONCE(prev->next, node);
>> =20
>>  		pv_wait_node(node, prev);
>> -		arch_mcs_spin_lock_contended(&node->locked);
>> +		/* Wait for mcs node lock to be released */
>> +		smp_cond_load_acquire(&node->locked, VAL);
>> =20
>>  		/*
>>  		 * While waiting for the MCS lock, the next pointer may have
>> @@ -554,7 +556,7 @@ void queued_spin_lock_slowpath(struct qspinlock *loc=
k, u32 val)
>>  	if (!next)
>>  		next =3D smp_cond_load_relaxed(&node->next, (VAL));
>> =20
>> -	arch_mcs_spin_unlock_contended(&next->locked);
>> +	smp_store_release(&next->locked, 1); /* unlock the mcs node lock */
>=20
> These are not equivalent. Now it so happens that ARM doesn't use
> qspinlock and the other mcs lock users are gone by now, but something
> like this should at least have been called out in the Changelog I think.
>=20

Yeah I could mention it in the changelog, I just didn't because it is
practically equivalent.

Thanks,
Nick