Re: [PATCH v6 4/5] arm64: futex: refactor futex atomic operation

[Yeoreum Yun <yeoreum.yun@arm.com>]

Hi Catalin,

[...]
> > diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h
> > index bc06691d2062..fdec4f3f2b15 100644
> > --- a/arch/arm64/include/asm/futex.h
> > +++ b/arch/arm64/include/asm/futex.h
> > @@ -7,73 +7,164 @@
> >
> >  #include <linux/futex.h>
> >  #include <linux/uaccess.h>
> > +#include <linux/stringify.h>
> >
> >  #include <asm/errno.h>
> >
> > -#define FUTEX_MAX_LOOPS	128 /* What's the largest number you can think of? */
> > +#define LLSC_MAX_LOOPS	128 /* What's the largest number you can think of? */
> >
> > -#define __futex_atomic_op(insn, ret, oldval, uaddr, tmp, oparg)		\
> > -do {									\
> > -	unsigned int loops = FUTEX_MAX_LOOPS;				\
> > +#define LLSC_FUTEX_ATOMIC_OP(op, asm_op)				\
> > +static __always_inline int						\
> > +__llsc_futex_atomic_##op(int oparg, u32 __user *uaddr, int *oval)	\
> > +{									\
> > +	unsigned int loops = LLSC_MAX_LOOPS;				\
> > +	int ret, val, tmp;						\
> >  									\
> >  	uaccess_enable_privileged();					\
> > -	asm volatile(							\
> > -"	prfm	pstl1strm, %2\n"					\
> > -"1:	ldxr	%w1, %2\n"						\
> > -	insn "\n"							\
> > -"2:	stlxr	%w0, %w3, %2\n"						\
> > -"	cbz	%w0, 3f\n"						\
> > -"	sub	%w4, %w4, %w0\n"					\
> > -"	cbnz	%w4, 1b\n"						\
> > -"	mov	%w0, %w6\n"						\
> > -"3:\n"									\
> > -"	dmb	ish\n"							\
> > +	asm volatile("// __llsc_futex_atomic_" #op "\n"		\
> > +	"	prfm	pstl1strm, %2\n"				\
> > +	"1:	ldxr	%w1, %2\n"					\
> > +	"	" #asm_op "	%w3, %w1, %w5\n"			\
> > +	"2:	stlxr	%w0, %w3, %2\n"					\
> > +	"	cbz	%w0, 3f\n"					\
> > +	"	sub	%w4, %w4, %w0\n"				\
> > +	"	cbnz	%w4, 1b\n"					\
> > +	"	mov	%w0, %w6\n"					\
> > +	"3:\n"								\
> > +	"	dmb	ish\n"						\
>
> Don't change indentation and code in the same patch, it makes it harder
> to follow what you actually changed. I guess the only difference is
> asm_op instead of insn.

Sorry for bothering you. I'll restore indentation to make it clear.
and yes. the only difference is to change you mention it.

>
> >  	_ASM_EXTABLE_UACCESS_ERR(1b, 3b, %w0)				\
> >  	_ASM_EXTABLE_UACCESS_ERR(2b, 3b, %w0)				\
> > -	: "=&r" (ret), "=&r" (oldval), "+Q" (*uaddr), "=&r" (tmp),	\
> > +	: "=&r" (ret), "=&r" (val), "+Q" (*uaddr), "=&r" (tmp),		\
>
> And here you changed oldval to val (was this necessary?)

Not really. I keep the "oldval" as it is.
Thanks.

>
> >  	  "+r" (loops)							\
> >  	: "r" (oparg), "Ir" (-EAGAIN)					\
> >  	: "memory");							\
> >  	uaccess_disable_privileged();					\
> > -} while (0)
> > +									\
> > +	if (!ret)							\
> > +		*oval = val;						\
> > +									\
> > +	return ret;							\
> > +}
> > +
> > +LLSC_FUTEX_ATOMIC_OP(add, add)
> > +LLSC_FUTEX_ATOMIC_OP(or, orr)
> > +LLSC_FUTEX_ATOMIC_OP(and, and)
> > +LLSC_FUTEX_ATOMIC_OP(eor, eor)
> > +
> > +static __always_inline int
> > +__llsc_futex_atomic_set(int oparg, u32 __user *uaddr, int *oval)
> > +{
> > +	unsigned int loops = LLSC_MAX_LOOPS;
> > +	int ret, val;
> > +
> > +	uaccess_enable_privileged();
> > +	asm volatile("//__llsc_futex_xchg\n"
> > +	"	prfm	pstl1strm, %2\n"
> > +	"1:	ldxr	%w1, %2\n"
> > +	"2:	stlxr	%w0, %w4, %2\n"
> > +	"	cbz	%w3, 3f\n"
> > +	"	sub	%w3, %w3, %w0\n"
> > +	"	cbnz	%w3, 1b\n"
> > +	"	mov	%w0, %w5\n"
> > +	"3:\n"
> > +	"	dmb	ish\n"
> > +	_ASM_EXTABLE_UACCESS_ERR(1b, 3b, %w0)
> > +	_ASM_EXTABLE_UACCESS_ERR(2b, 3b, %w0)
> > +	: "=&r" (ret), "=&r" (val), "+Q" (*uaddr), "+r" (loops)
> > +	: "r" (oparg), "Ir" (-EAGAIN)
> > +	: "memory");
> > +	uaccess_disable_privileged();
>
> Was this separate function just to avoid the "mov" instruction for the
> "set" case? The patch description states that the reworking is necessary
> for the FEAT_LSUI use but it looks to me like it does more. Please split
> it in separate patches, though I'd leave any potential optimisation for
> a separate series and keep the current code as close as possible to the
> original one.
>

Yes. It's a small optimisation -- not use "mov" instruction.
I'll separate that part.

Thanks!

--
Sincerely,
Yeoreum Yun