On 7/6/2022 8:03 PM, Boqun Feng wrote: > On Sat, Jun 25, 2022 at 01:29:50PM +0800, Guo Ren wrote: >> On Fri, Jun 24, 2022 at 1:09 AM Dan Lustig <dlustig@xxxxxxxxxx> wrote: >>> >>> On 6/22/2022 11:31 PM, Boqun Feng wrote: >>>> Hi, >>>> >>>> On Tue, Jun 14, 2022 at 01:03:47PM +0200, Andrea Parri wrote: >>>> [...] >>>>>> 5ce6c1f3535f ("riscv/atomic: Strengthen implementations with fences") >>>>>> is about fixup wrong spinlock/unlock implementation and not relate to >>>>>> this patch. >>>>> >>>>> No. The commit in question is evidence of the fact that the changes >>>>> you are presenting here (as an optimization) were buggy/incorrect at >>>>> the time in which that commit was worked out. >>>>> >>>>> >>>>>> Actually, sc.w.aqrl is very strong and the same with: >>>>>> fence rw, rw >>>>>> sc.w >>>>>> fence rw,rw >>>>>> >>>>>> So "which do not give full-ordering with .aqrl" is not writen in >>>>>> RISC-V ISA and we could use sc.w/d.aqrl with LKMM. >>>>>> >>>>>>> >>>>>>>>> describes the issue more specifically, that's when we added these >>>>>>>>> fences. There have certainly been complains that these fences are too >>>>>>>>> heavyweight for the HW to go fast, but IIUC it's the best option we have >>>>>>>> Yeah, it would reduce the performance on D1 and our next-generation >>>>>>>> processor has optimized fence performance a lot. >>>>>>> >>>>>>> Definately a bummer that the fences make the HW go slow, but I don't >>>>>>> really see any other way to go about this. If you think these mappings >>>>>>> are valid for LKMM and RVWMO then we should figure this out, but trying >>>>>>> to drop fences to make HW go faster in ways that violate the memory >>>>>>> model is going to lead to insanity. >>>>>> Actually, this patch is okay with the ISA spec, and Dan also thought >>>>>> it was valid. >>>>>> >>>>>> ref: https://lore.kernel.org/lkml/41e01514-74ca-84f2-f5cc-2645c444fd8e@xxxxxxxxxx/raw >>>>> >>>>> "Thoughts" on this regard have _changed_. Please compare that quote >>>>> with, e.g. >>>>> >>>>> https://lore.kernel.org/linux-riscv/ddd5ca34-805b-60c4-bf2a-d6a9d95d89e7@xxxxxxxxxx/ >>>>> >>>>> So here's a suggestion: >>>>> >>>>> Reviewers of your patches have asked: How come that code we used to >>>>> consider as buggy is now considered "an optimization" (correct)? >>>>> >>>>> Denying the evidence or going around it is not making their job (and >>>>> this upstreaming) easier, so why don't you address it? Take time to >>>>> review previous works and discussions in this area, understand them, >>>>> and integrate such knowledge in future submissions. >>>>> >>>> >>>> I agree with Andrea. >>>> >>>> And I actually took a look into this, and I think I find some >>>> explanation. There are two versions of RISV memory model here: >>>> >>>> Model 2017: released at Dec 1, 2017 as a draft >>>> >>>> https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/hKywNHBkAXM/m/QzUtxEWLBQAJ >>>> >>>> Model 2018: released at May 2, 2018 >>>> >>>> https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/xW03vmfmPuA/m/bMPk3UCWAgAJ >>>> >>>> Noted that previous conversation about commit 5ce6c1f3535f happened at >>>> March 2018. So the timeline is roughly: >>>> >>>> Model 2017 -> commit 5ce6c1f3535f -> Model 2018 >>>> >>>> And in the email thread of Model 2018, the commit related to model >>>> changes also got mentioned: >>>> >>>> https://github.com/riscv/riscv-isa-manual/commit/b875fe417948635ed68b9644ffdf718cb343a81a >>>> >>>> in that commit, we can see the changes related to sc.aqrl are: >>>> >>>> to have occurred between the LR and a successful SC. The LR/SC >>>> sequence can be given acquire semantics by setting the {\em aq} bit on >>>> -the SC instruction. The LR/SC sequence can be given release semantics >>>> -by setting the {\em rl} bit on the LR instruction. Setting both {\em >>>> - aq} and {\em rl} bits on the LR instruction, and setting the {\em >>>> - aq} bit on the SC instruction makes the LR/SC sequence sequentially >>>> -consistent with respect to other sequentially consistent atomic >>>> -operations. >>>> +the LR instruction. The LR/SC sequence can be given release semantics >>>> +by setting the {\em rl} bit on the SC instruction. Setting the {\em >>>> + aq} bit on the LR instruction, and setting both the {\em aq} and the {\em >>>> + rl} bit on the SC instruction makes the LR/SC sequence sequentially >>>> +consistent, meaning that it cannot be reordered with earlier or >>>> +later memory operations from the same hart. >>>> >>>> note that Model 2018 explicitly says that "ld.aq+sc.aqrl" is ordered >>>> against "earlier or later memory operations from the same hart", and >>>> this statement was not in Model 2017. >>>> >>>> So my understanding of the story is that at some point between March and >>>> May 2018, RISV memory model folks decided to add this rule, which does >>>> look more consistent with other parts of the model and is useful. >>>> >>>> And this is why (and when) "ld.aq+sc.aqrl" can be used as a fully-ordered >>>> barrier ;-) >>>> >>>> Now if my understanding is correct, to move forward, it's better that 1) >>>> this patch gets resend with the above information (better rewording a >>>> bit), and 2) gets an Acked-by from Dan to confirm this is a correct >>>> history ;-) >>> >>> I'm a bit lost as to why digging into RISC-V mailing list history is >>> relevant here...what's relevant is what was ratified in the RVWMO >>> chapter of the RISC-V spec, and whether the code you're proposing >>> is the most optimized code that is correct wrt RVWMO. >>> >>> Is your claim that the code you're proposing to fix was based on a >>> pre-RVWMO RISC-V memory model definition, and you're updating it to >>> be more RVWMO-compliant? >> Could "lr + beq + sc.aqrl" provides a conditional RCsc here with >> current spec? I only found "lr.aq + sc.aqrl" despcriton which is >> un-conditional RCsc. >> > > /me put the temporary RISCV memory model hat on and pretend to be a > RISCV memory expert. > > I think the answer is yes, it's actually quite straightforwards given > that RISCV treats PPO (Preserved Program Order) as part of GMO (Global > Memory Order), considering the following (A and B are memory accesses): > > A > .. > sc.aqrl // M > .. > B > > , A has a ->ppo ordering to M since "sc.aqrl" is a RELEASE, and M has > a ->ppo ordeing to B since "sc.aqrl" is an AQUIRE, so > > A ->ppo M ->ppo B > > And since RISCV describes that PPO is part of GMO: > > """ > The subset of program order that must be respected by the global memory > order is known as preserved program order. > """ > > also in the herd model: > > (* Main model axiom *) > acyclic co | rfe | fr | ppo as Model > > , therefore the ordering between A and B is GMO and GMO should be > respected by all harts. > > Regards, > Boqun I agree with Boqun's reasoning, at least for the case where there is no branch. But to confirm, was the original question about also having a branch, I assume to the instruction immediately after the sc? If so, then yes, that would make the .aqrl effect conditional. Dan > >>> >>> Dan >>> >>>> Regards, >>>> Boqun >>>> >>>>> Andrea >>>>> >>>>> >>>> [...] >> >> >> >> -- >> Best Regards >> Guo Ren >> >> ML: https://lore.kernel.org/linux-csky/