Re: [PATCH RFC 1/2] arch: Introduce ARCH_HAS_HW_XCHG_SMALL

From: Jiaxun Yang <jiaxun.yang@flygoat.com>
To: Arnd Bergmann <arnd@arndb.de>, Huacai Chen <chenhuacai@loongson.cn>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
	Waiman Long <longman@redhat.com>,
	Boqun Feng <boqun.feng@gmail.com>, Guo Ren <guoren@kernel.org>,
	linux-arch <linux-arch@vger.kernel.org>,
	Rui Wang <wangrui@loongson.cn>,
	Xuefeng Li <lixuefeng@loongson.cn>,
	Huacai Chen <chenhuacai@gmail.com>
Subject: Re: [PATCH RFC 1/2] arch: Introduce ARCH_HAS_HW_XCHG_SMALL
Date: Sun, 25 Jul 2021 11:06:10 +0800	[thread overview]
Message-ID: <ec727497-7fe9-a52c-3063-ccd0459159fe@flygoat.com> (raw)
In-Reply-To: <CAK8P3a0ZabB0cBR_SnOhi2=qxdQOYPGPEJeOqV0em1+bsvZKWw@mail.gmail.com>

在 2021/7/25 上午3:24, Arnd Bergmann 写道:
> On Sat, Jul 24, 2021 at 2:36 PM Huacai Chen <chenhuacai@loongson.cn> wrote:
>> Introduce a new Kconfig option ARCH_HAS_HW_XCHG_SMALL, which means arch
>> has hardware sub-word xchg/cmpxchg support. This option will be used as
>> an indicator to select the bit-field definition in the qspinlock data
>> structure.
>>
>> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> Adding a Kconfig conditional sounds like a good idea, but I have two
> concerns about the specific implementation:
>
> - I think we should have separate symbols for 8-bit, 16-bit and 64-bit
>    cmpxchg(). I think every architecture needs to support at least 32-bit
>    cmpxchg() and 64-bit architectures also need to support cmpxchg64().
>    I actually have a prototype patch that introduces cmpxchg8() and
>    cmpxchg16() helpers with the purpose of no longer supporting these
>    width in the normal cmpxchg(), but that is mostly independent of
>    whether we want a conditional or not.
Yeah that seems a better solution to make supporting status clear,

>
> - If I remember correctly, there were some concerns about whether using
>    this information for picking the qspinlock implementation is a good idea.
We've checked previous attempt made by Guo Ren about
ARCH_USE_QUEUED_SPINLOCKS_XCHG32[1], the concerns of potential livelock 
do exist.

So in this patch Huacai took another, dropping the whole standalone 
tailing logic to remove
the usage of sub-word xchg. It could be understood as partial revert of  
69f9cae9 ("
locking/qspinlock: Optimize for smaller NR_CPUS") [2] on these 
architectures.

>
>           Arnd
[1]: 
https://lore.kernel.org/linux-csky/1617201040-83905-2-git-send-email-guoren@kernel.org/
[2]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=69f9cae90907e09af95fb991ed384670cef8dd32

Thanks.

- Jiaxun