Re: [PATCH v2 2/2] riscv/atomic: Strengthen implementations with fences

[Luc Maranget]

> On Fri, Mar 09, 2018 at 04:21:37PM -0800, Daniel Lustig wrote:
> > On 3/9/2018 2:57 PM, Palmer Dabbelt wrote:
> > > On Fri, 09 Mar 2018 13:30:08 PST (-0800), parri.andrea@xxxxxxxxx wrote:
> > >> On Fri, Mar 09, 2018 at 10:54:27AM -0800, Palmer Dabbelt wrote:
> > >>> On Fri, 09 Mar 2018 10:36:44 PST (-0800), parri.andrea@xxxxxxxxx wrote:
> > >>
> > >> [...]
> > >>
> > >>> >This proposal relies on the generic definition,
> > >>> >
> > >>> >ÂÂ include/linux/atomic.h ,
> > >>> >
> > >>> >and on the
> > >>> >
> > >>> >ÂÂ __atomic_op_acquire()
> > >>> >ÂÂ __atomic_op_release()
> > >>> >
> > >>> >above to build the acquire/release atomics (except for the xchg,cmpxchg,
> > >>> >where the ACQUIRE_BARRIER is inserted conditionally/on success).
> > >>>
> > >>> I thought we wanted to use the AQ and RL bits for AMOs, just not for LR/SC
> > >>> sequences. IIRC the AMOs are safe with the current memory model, but I might
> > >>> just have some version mismatches in my head.
> > >>
> > >> AMO.aqrl are "safe" w.r.t. the LKMM (as they provide "full-ordering"); OTOH,
> > >> AMO.aq and AMO.rl present weaknesses that LKMM (and some kernel developers)
> > >> do not "expect". I was probing this issue in:
> > >>
> > >> Â https://marc.info/?l=linux-kernel&m=151930201102853&w=2
> > >>
> > >> (c.f., e.g., test "RISCV-unlock-lock-read-ordering" from that post).
> > >>
> > >> Quoting from the commit message of my patch 1/2:
> > >>
> > >> Â "Referring to the "unlock-lock-read-ordering" test reported below,
> > >> ÂÂ Daniel wrote:
> > >>
> > >> ÂÂÂÂ I think an RCpc interpretation of .aq and .rl would in fact
> > >> ÂÂÂÂ allow the two normal loads in P1 to be reordered [...]
> > >>
> > >> ÂÂÂÂ [...]
> > >>
> > >> ÂÂÂÂ Likewise even if the unlock()/lock() is between two stores.
> > >> ÂÂÂÂ A control dependency might originate from the load part of
> > >> ÂÂÂÂ the amoswap.w.aq, but there still would have to be something
> > >> ÂÂÂÂ to ensure that this load part in fact performs after the store
> > >> ÂÂÂÂ part of the amoswap.w.rl performs globally, and that's not
> > >> ÂÂÂÂ automatic under RCpc.
> > >>
> > >> ÂÂ Simulation of the RISC-V memory consistency model confirmed this
> > >> ÂÂ expectation."
> > >>
> > >> I have just (re)checked these observations against the latest specification,
> > >> and my results _confirmed_ these verdicts.
> > > 
> > > Thanks, I must have just gotten confused about a draft spec or something. I'm
> > > pulling these on top or your other memory model related patch. I've renamed
> > > the branch "next-mm" to be a bit more descriptiove.
> > 
> > (Sorry for being out of the loop this week, I was out to deal with
> > a family matter.)
> > 
> > I assume you're using the herd model?  Luc's doing a great job with
> > that, but even so, nothing is officially confirmed until we ratify
> > the model.  In other words, the herd model may end up changing too.
> > If something is broken on our end, there's still time to fix it.
> > 
> > Regarding AMOs, let me copy from something I wrote in a previous
> > offline conversation:
> > 
> > > it seems to us that pairing a store-release of "amoswap.rl" with
> > > a "ld; fence r,rw" doesn't actually give us the RC semantics we've
> > > been discussing for LKMM.  For example:
> > > 
> > > (a) sd t0,0(s0)
> > > (b) amoswap.d.rl x0,t1,0(s1)
> > >     ...
> > > (c) ld a0,0(s1)
> > > (d) fence r,rw
> > > (e) sd t2,0(s2)
> > > 
> > > There, we won't get (a) ordered before (e) regardless of whether
> > > (b) is RCpc or RCsc.  Do you agree?
> > 
> > At the moment, only the load part of (b) is in the predecessor
> > set of (d), but the store part of (b) is not.  Likewise, the
> > .rl annotation applies only to the store part of (b), not the
> > load part.
> > 
> > This gets back to a question Linus asked last week about
> > whether the AMO is a single unit or whether it can be
> 
> You mean AMO or RmW atomic operations?
> 
> > considered to split into a load and a store part (which still
> > perform atomically).  For RISC-V, for right now at least, the
> > answer is the latter.  Is it still the latter for Linux too?
> > 
> 
> I think for RmW atomics it's still the latter, the acquire or release is
> for the load part or the store part of an RmW. For example, ppc uses
> lwsync as acquire/release barriers, and lwsync could not order
> write->read. 

You are correct LKMM represent read-modify-write constructs with 2 events,
one read and one write. Those are connected by the special "rmw" relation.



> 
> Regards,
> Boqun

Regards,

--Luc


> 
> > https://lkml.org/lkml/2018/2/26/606
> > 
> > > So I think we'll need to make sure we pair .rl with .aq, or that
> > > we pair fence-based mappings with fence-based mappings, in order
> > > to make the acquire/release operations work.
> > 
> > This assumes we'll say that .aq and .rl are RCsc, not RCpc.
> > But in this case, I think .aq and .rl could still be safe to use,
> > as long as you don't ever try to mix in a fence-based mapping
> > on the same data structure like in the example above.  That
> > might be important if we want to find the most compact legal
> > implementation, and hence do want to use .aq and .rl after all.
> > 
> > > And since we don't have native "ld.aq" today in RISC-V, that
> > > would mean smp_store_release would have to remain implemented
> > > as "fence rw,w; s{w|d}", rather than "amoswap.{w|d}.rl", for
> > > example.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Dan
> > 
> > > 
> > > Thanks!