Re: [PATCH RESEND v7 4/6] arm64: futex: refactor futex atomic operation

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

Hi Will,

[...]

> > -#define __futex_atomic_op(insn, ret, oldval, uaddr, tmp, oparg)		\
> > -do {									\
> > -	unsigned int loops = FUTEX_MAX_LOOPS;				\
> > +#define LLSC_FUTEX_ATOMIC_OP(op, insn)					\
> > +static __always_inline int						\
> > +__llsc_futex_atomic_##op(int oparg, u32 __user *uaddr, int *oval)	\
> > +{									\
> > +	unsigned int loops = LLSC_MAX_LOOPS;				\
> > +	int ret, oldval, tmp;						\
> >  									\
> >  	uaccess_enable_privileged();					\
> > -	asm volatile(							\
> > +	asm volatile("// __llsc_futex_atomic_" #op "\n"			\
> >  "	prfm	pstl1strm, %2\n"					\
> >  "1:	ldxr	%w1, %2\n"						\
> >  	insn "\n"							\
> > @@ -35,45 +39,103 @@ do {									\
> >  	: "r" (oparg), "Ir" (-EAGAIN)					\
> >  	: "memory");							\
> >  	uaccess_disable_privileged();					\
> > -} while (0)
> > +									\
> > +	if (!ret)							\
> > +		*oval = oldval;						\
>
> Why push the store to '*oval' down into here?

As __llsc_futext_atomic_##op() is declared with inline function
I think it would be better to pass oval from arch_futex_atomic_op_inuser()
as it is for readability.

Is it awful?

Thanks.

--
Sincerely,
Yeoreum Yun