Messages in this thread | | | From | Paul Burton <> | Subject | Re: [RFC][PATCH 2/5] mips/atomic: Fix loongson_llsc_mb() wreckage | Date | Wed, 24 Apr 2019 21:18:04 +0000 |
| |
Hi Peter,
On Wed, Apr 24, 2019 at 02:36:58PM +0200, Peter Zijlstra wrote: > The comment describing the loongson_llsc_mb() reorder case doesn't > make any sense what so ever. Instruction re-ordering is not an SMP > artifact, but rather a CPU local phenomenon. This means that _every_ > LL/SC loop needs this barrier right in front to avoid the CPU from > leaking a memop inside it.
Does it?
The Loongson bug being described here causes an sc to succeed erroneously if certain loads or stores are executed between the ll & associated sc, including speculatively. On a UP system there's no code running on other cores to race with us & cause our sc to fail - ie. sc should always succeed anyway, so if the bug hits & the sc succeeds what's the big deal? It would have succeeded anyway. At least that's my understanding based on discussions with Loongson engineers a while ago.
Having said that, if you have a strong preference for adding the barrier in UP systems anyway then I don't really object. It's not like anyone's likely to want to run a UP kernel on the affected systems, nevermind care about a miniscule performance impact.
One possibility your change could benefit would be if someone ran Linux on a subset of cores & some non-Linux code on other cores, in which case there could be something to cause the sc to fail. I've no idea if that's something these Loongson systems ever do though.
> For the branch speculation case; if futex_atomic_cmpxchg_inatomic() > needs one at the bne branch target, then surely the normal > __cmpxch_asmg() implementation does too. We cannot rely on the
s/cmpxch_asmg/cmpxchg_asm/
> barriers from cmpxchg() because cmpxchg_local() is implemented with > the same macro, and branch prediction and speculation are, too, CPU > local.
Similar story - cmpxchg_local() ought not have have CPUs racing for access to the memory in question. Having said that I don't know the details of when Loongson clears LLBit (ie. causes an sc to fail), so if it does that on based upon access to memory at a larger granularity than the 32b or 64b value being operated on then that could be a problem so I'm pretty happy with adding these barriers.
Thanks, Paul
| |