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

[éåæ]

Hi, Paul,

First of all, could you please check why linux-mips reject e-mails from lemote.com? Of course I can send e-mails by gmail, but my gmail can't receive e-mails from linux-mips since March, 2018.

I have already read Documentation/memory-barriers.txt, but I don't think we should define a smp_read_barrier_depends() for Loongson-3. Because Loongson-3's behavior isn't like Alpha, and in syntax, this is not a data-dependent issue.

There is no document about Loongson-3's SFB. In my opinion, SFB looks like the L0 cache but sometimes it is out of cache-coherent machanism (L1 cache's cross-core coherency is maintained by hardware, but not always true for SFB). smp_mb() is needed for smp_cond_load_acquire(), but not every READ_ONCE().

Huacai
 
------------------ Original ------------------
From:  "Paul Burton"<paul.burton@xxxxxxxx>;
Date:  Tue, Jun 19, 2018 02:51 AM
To:  "Huacai Chen"<chenhc@xxxxxxxxxx>;
Cc:  "Ralf Baechle"<ralf@xxxxxxxxxxxxxx>; "James Hogan"<james.hogan@xxxxxxxx>; "linux-mips"<linux-mips@xxxxxxxxxxxxxx>; "Fuxin Zhang"<zhangfx@xxxxxxxxxx>; "wuzhangjin"<wuzhangjin@xxxxxxxxx>; "Huacai Chen"<chenhuacai@xxxxxxxxx>; "stable"<stable@xxxxxxxxxxxxxxx>; "Alan Stern"<stern@xxxxxxxxxxxxxxxxxxx>; "AndreaParri"<andrea.parri@xxxxxxxxxxxxxxxxxxxx>; "Will Deacon"<will.deacon@xxxxxxx>; "Peter Zijlstra"<peterz@xxxxxxxxxxxxx>; "Boqun Feng"<boqun.feng@xxxxxxxxx>; "Nicholas Piggin"<npiggin@xxxxxxxxx>; "David Howells"<dhowells@xxxxxxxxxx>; "Jade Alglave"<j.alglave@xxxxxxxxx>; "Luc Maranget"<luc.maranget@xxxxxxxx>; "Paul E. McKenney"<paulmck@xxxxxxxxxxxxxxxxxx>; "Akira Yokosawa"<akiyks@xxxxxxxxx>; "linux-kernel"<linux-kernel@xxxxxxxxxxxxxxx>;
Subject:  Re: [PATCH] MIPS: implement smp_cond_load_acquire() for Loongson-3
 
Hi Huacai,

On Fri, Jun 15, 2018 at 02:07:38PM +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 READ_ONCE() may get an old value in a
> tight loop. So in smp_cond_load_acquire() we need a __smp_mb() after
> every READ_ONCE().

Thanks - modifying smp_cond_load_acquire() is a step better than
modifying arch_mcs_spin_lock_contended() to avoid it, but I'm still not
sure we've reached the root of the problem. If tight loops using
READ_ONCE() are at fault then what's special about
smp_cond_load_acquire()? Could other such loops not hit the same
problem?

Is the scenario you encounter the same as that outlined in the "DATA
DEPENDENCY BARRIERS (HISTORICAL)" section of
Documentation/memory-barriers.txt by any chance? If so then perhaps it
would be better to implement smp_read_barrier_depends(), or just raw
read_barrier_depends() depending upon how your SFB functions.

Is there any public documentation describing the behaviour of the store
fill buffer in Loongson-3?

Part of the problem is that I'm still not sure what's actually happening
in your system - it would be helpful to have further information in the
commit message about what actually happens. For example if you could
walk us through an example of the problem step by step in the style of
the diagrams you'll see in Documentation/memory-barriers.txt then I
think that would help us to see what the best solution here is.

I've copied the LKMM maintainers in case they have further input.

Thanks,
    Paul

> 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.
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Huacai Chen <chenhc@xxxxxxxxxx>
> ---
>  arch/mips/include/asm/barrier.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/mips/include/asm/barrier.h b/arch/mips/include/asm/barrier.h
> index a5eb1bb..4ea384d 100644
> --- a/arch/mips/include/asm/barrier.h
> +++ b/arch/mips/include/asm/barrier.h
> @@ -222,6 +222,23 @@
>  #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_mb() after READ_ONCE() here */
> +#define smp_cond_load_acquire(ptr, cond_expr)	\
> +({	\
> +	typeof(ptr) __PTR = (ptr);	\
> +	typeof(*ptr) VAL;	\
> +	for (;;) {	\
> +	VAL = READ_ONCE(*__PTR);	\
> +	__smp_mb();	\
> +	if (cond_expr)	\
> +	break;	\
> +	cpu_relax();	\
> +	}	\
> +	VAL;	\
> +})
> +#endif	/* CONFIG_CPU_LOONGSON3 */
> +
>  #include <asm-generic/barrier.h>
>  
>  #endif /* __ASM_BARRIER_H */
> -- 
> 2.7.0
>