Re: [PATCH V2] MIPS: implement smp_cond_load_acquire() for Loongson-3

From: Paul Burton <paul.burton@mips.com>
To: Huacai Chen <chenhc@lemote.com>
Cc: Ralf Baechle <ralf@linux-mips.org>,
	James Hogan <james.hogan@mips.com>,
	linux-mips@linux-mips.org, Fuxin Zhang <zhangfx@lemote.com>,
	Zhangjin Wu <wuzhangjin@gmail.com>,
	Huacai Chen <chenhuacai@gmail.com>,
	stable@vger.kernel.org, Alan Stern <stern@rowland.harvard.edu>,
	Andrea Parri <andrea.parri@amarulasolutions.com>,
	Will Deacon <will.deacon@arm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Boqun Feng <boqun.feng@gmail.com>,
	Nicholas Piggin <npiggin@gmail.com>,
	David Howells <dhowells@redhat.com>,
	Jade Alglave <j.alglave@ucl.ac.uk>,
	Luc Maranget <luc.maranget@inria.fr>,
	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
	Akira Yokosawa <akiyks@gmail.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2] MIPS: implement smp_cond_load_acquire() for Loongson-3
Date: Mon, 9 Jul 2018 09:49:39 -0700	[thread overview]
Message-ID: <20180709164939.uhqsvcv4a7jlbhvp@pburton-laptop> (raw)
In-Reply-To: <1531103198-16764-1-git-send-email-chenhc@lemote.com>

Hi Huacai,

On Mon, Jul 09, 2018 at 10:26:38AM +0800, Huacai Chen wrote:
> After commit 7f56b58a92aaf2c ("locking/mcs: Use smp_cond_load_acquire()
> in MCS spin loop") Loongson-3 fails to boot. This is because Loongson-3
> has SFB (Store Fill Buffer) and the weak-ordering may cause READ_ONCE()
> to get an old value in a tight loop. So in smp_cond_load_acquire() we
> need a __smp_rmb() before the READ_ONCE() loop.
> 
> This patch introduce a Loongson-specific smp_cond_load_acquire(). And
> it should be backported to as early as linux-4.5, in which release the
> smp_cond_acquire() is introduced.
> 
> There may be other cases where memory barriers is needed, we will fix
> them one by one.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
>  arch/mips/include/asm/barrier.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/arch/mips/include/asm/barrier.h b/arch/mips/include/asm/barrier.h
> index a5eb1bb..e8c4c63 100644
> --- a/arch/mips/include/asm/barrier.h
> +++ b/arch/mips/include/asm/barrier.h
> @@ -222,6 +222,24 @@
>  #define __smp_mb__before_atomic()	__smp_mb__before_llsc()
>  #define __smp_mb__after_atomic()	smp_llsc_mb()
>  
> +#ifdef CONFIG_CPU_LOONGSON3
> +/* Loongson-3 need a __smp_rmb() before READ_ONCE() loop */
> +#define smp_cond_load_acquire(ptr, cond_expr)			\
> +({								\
> +	typeof(ptr) __PTR = (ptr);				\
> +	typeof(*ptr) VAL;					\
> +	__smp_rmb();						\
> +	for (;;) {						\
> +		VAL = READ_ONCE(*__PTR);			\
> +		if (cond_expr)					\
> +			break;					\
> +		cpu_relax();					\
> +	}							\
> +	__smp_rmb();						\
> +	VAL;							\
> +})
> +#endif	/* CONFIG_CPU_LOONGSON3 */

The discussion on v1 of this patch [1] seemed to converge on the view
that Loongson suffers from the same problem as ARM platforms which
enable the CONFIG_ARM_ERRATA_754327 workaround, and that we might
require a similar workaround.

Is there a reason you've not done that, and instead tweaked your patch
that's specific to the smp_cond_load_acquire() case? I'm not comfortable
with fixing just this one case when there could be many similar
problematic pieces of code you just haven't hit yet.

Please also keep the LKMM maintainers on copy for this - their feedback
will be valuable & I'll be much more comfortable applying a workaround
for Loongson's behavior here if it's one that they're OK with.

Thanks,
    Paul

[1] https://www.linux-mips.org/archives/linux-mips/2018-06/msg00139.html