Received-SPF: pass (google.com: domain of linux-kernel+bounces-176728-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Leonardo Bras <leobras@redhat.com>
To: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Leonardo Bras <leobras@redhat.com>,
	Guo Ren <guoren@kernel.org>,
	linux-kernel@vger.kernel.org,
	kernel-team@meta.com,
	andi.shyti@linux.intel.com,
	andrzej.hajda@intel.com,
	linux-riscv@lists.infradead.org,
	palmer@dabbelt.com
Subject: Re: [PATCH RFC cmpxchg 8/8] riscv: Emulate one-byte and two-byte cmpxchg
Date: Sat, 11 May 2024 17:44:46 -0300
Message-ID: <Zj_YvmjROzW6NAog@LeoBras>
In-Reply-To: <9a84b94c-34ff-4c3a-ab2b-2741a5755db9@paulmck-laptop>
References: <20240401213950.3910531-8-paulmck@kernel.org> <mhng-d6a8a972-5054-4c48-a903-5a53a31da9ad@palmer-ri-x1c9a> <Zj8VJZmQC7hRzDdz@gmail.com> <9a84b94c-34ff-4c3a-ab2b-2741a5755db9@paulmck-laptop>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Sat, May 11, 2024 at 07:54:34AM -0700, Paul E. McKenney wrote:
> On Sat, May 11, 2024 at 02:50:13AM -0400, Guo Ren wrote:
> > On Thu, Apr 04, 2024 at 07:15:40AM -0700, Palmer Dabbelt wrote:
> > > On Mon, 01 Apr 2024 14:39:50 PDT (-0700), paulmck@kernel.org wrote:
> > > > Use the new cmpxchg_emu_u8() and cmpxchg_emu_u16() to emulate one-byte
> > > > and two-byte cmpxchg() on riscv.
> > > > 
> > > > [ paulmck: Apply kernel test robot feedback. ]
> > > 
> > > I'm not entirely following the thread, but sounds like there's going to be
> > > generic kernel users of this now?  Before we'd said "no" to the byte/half
> > > atomic emulation routines beacuse they weren't used, but if it's a generic
> > > thing then I'm find adding them.
> > > 
> > > There's a patch set over here
> > > <https://lore.kernel.org/all/20240103163203.72768-2-leobras@redhat.com/>
> > > that implements these more directly using LR/SC.  I was sort of on the fence
> > > about just taking it even with no direct users right now, as the byte/half
> > > atomic extension is working its way through the spec process so we'll have
> > > them for real soon.  I stopped right there for the last merge window,
> > > though, as I figured it was too late to be messing with the atomics...
> > > 
> > > So
> > > 
> > > Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
> > F.Y.I Leonardo Bras <leobras@redhat.com>

Hi Guo Ren, thanks for bringing it to my attention.

I am quite excited about the inclusion of my patchset on riscv/cmpxchg, and 
I hope it can be useful both to your qspinlock implementation and on Paul's 
RCU improvement.

> 
> I am carrying this in -rcu, but only for testing purposes, not for
> inclusion into mainline.  Not that I know of anyone testing -rcu on
> RISC-V, but still, I wouldn't want to do anything do discourage such
> testing.
> 
> The reason that this patch is no longer intended for inclusion is that it
> has been obsoleted by a patch that provides native support for one-byte
> and two-byte cmpxchg() operations.  Which is even better!  ;-)
> 
> 							Thanx, Paul

Thanks Paul!
Months ago I have reworked cmpxchg and added those 1-byte and 2-byte 
{cmp,}xchg asm implementations using lr/sc, as they would be useful to Guo 
Ren's qspinlock, and I am thankful that you provided another use case, 
because it provides more proof of it's usefulness.

Thanks!
Leo

> 
> > > if you guys want to take some sort of tree-wide change to make the byte/half
> > > stuff be required everywhere.  We'll eventually end up with arch routines
> > > for the extension, so at that point we might as well also have the more
> > > direct LR/SC flavors.
> > > 
> > > If you want I can go review/merge that RISC-V patch set and then it'll have
> > > time to bake for a shared tag you can pick up for all this stuff?  No rush
> > > on my end, just LMK.
> > > 
> > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > Cc: Andi Shyti <andi.shyti@linux.intel.com>
> > > > Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> > > > Cc: <linux-riscv@lists.infradead.org>
> > > > ---
> > > >  arch/riscv/Kconfig               |  1 +
> > > >  arch/riscv/include/asm/cmpxchg.h | 25 +++++++++++++++++++++++++
> > > >  2 files changed, 26 insertions(+)
> > > > 
> > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > index be09c8836d56b..4eaf40d0a52ec 100644
> > > > --- a/arch/riscv/Kconfig
> > > > +++ b/arch/riscv/Kconfig
> > > > @@ -44,6 +44,7 @@ config RISCV
> > > >  	select ARCH_HAS_UBSAN
> > > >  	select ARCH_HAS_VDSO_DATA
> > > >  	select ARCH_KEEP_MEMBLOCK if ACPI
> > > > +	select ARCH_NEED_CMPXCHG_1_2_EMU
> > > >  	select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
> > > >  	select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT
> > > >  	select ARCH_STACKWALK
> > > > diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> > > > index 2fee65cc84432..a5b377481785c 100644
> > > > --- a/arch/riscv/include/asm/cmpxchg.h
> > > > +++ b/arch/riscv/include/asm/cmpxchg.h
> > > > @@ -9,6 +9,7 @@
> > > >  #include <linux/bug.h>
> > > > 
> > > >  #include <asm/fence.h>
> > > > +#include <linux/cmpxchg-emu.h>
> > > > 
> > > >  #define __xchg_relaxed(ptr, new, size)					\
> > > >  ({									\
> > > > @@ -170,6 +171,12 @@
> > > >  	__typeof__(*(ptr)) __ret;					\
> > > >  	register unsigned int __rc;					\
> > > >  	switch (size) {							\
> > > > +	case 1:								\
> > > > +		__ret = cmpxchg_emu_u8((volatile u8 *)__ptr, __old, __new); \
> > > > +		break;							\
> > > > +	case 2:								\
> > > > +		break;							\
> > > > +		__ret = cmpxchg_emu_u16((volatile u16 *)__ptr, __old, __new); \
> > > >  	case 4:								\
> > > >  		__asm__ __volatile__ (					\
> > > >  			"0:	lr.w %0, %2\n"				\
> > > > @@ -214,6 +221,12 @@
> > > >  	__typeof__(*(ptr)) __ret;					\
> > > >  	register unsigned int __rc;					\
> > > >  	switch (size) {							\
> > > > +	case 1:								\
> > > > +		__ret = cmpxchg_emu_u8((volatile u8 *)__ptr, __old, __new); \
> > > > +		break;							\
> > > > +	case 2:								\
> > > > +		break;							\
> > > > +		__ret = cmpxchg_emu_u16((volatile u16 *)__ptr, __old, __new); \
> > > >  	case 4:								\
> > > >  		__asm__ __volatile__ (					\
> > > >  			"0:	lr.w %0, %2\n"				\
> > > > @@ -260,6 +273,12 @@
> > > >  	__typeof__(*(ptr)) __ret;					\
> > > >  	register unsigned int __rc;					\
> > > >  	switch (size) {							\
> > > > +	case 1:								\
> > > > +		__ret = cmpxchg_emu_u8((volatile u8 *)__ptr, __old, __new); \
> > > > +		break;							\
> > > > +	case 2:								\
> > > > +		break;							\
> > > > +		__ret = cmpxchg_emu_u16((volatile u16 *)__ptr, __old, __new); \
> > > >  	case 4:								\
> > > >  		__asm__ __volatile__ (					\
> > > >  			RISCV_RELEASE_BARRIER				\
> > > > @@ -306,6 +325,12 @@
> > > >  	__typeof__(*(ptr)) __ret;					\
> > > >  	register unsigned int __rc;					\
> > > >  	switch (size) {							\
> > > > +	case 1:								\
> > > > +		__ret = cmpxchg_emu_u8((volatile u8 *)__ptr, __old, __new); \
> > > > +		break;							\
> > > > +	case 2:								\
> > > > +		break;							\
> > > > +		__ret = cmpxchg_emu_u16((volatile u16 *)__ptr, __old, __new); \
> > > >  	case 4:								\
> > > >  		__asm__ __volatile__ (					\
> > > >  			"0:	lr.w %0, %2\n"				\
> > > 
>