On 7/13/2022 7:47 PM, Guo Ren wrote: > On Thu, Jul 7, 2022 at 8:04 AM Boqun Feng <boqun.feng@xxxxxxxxx> 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 > That also means M must fence.rl + sc + fence.aq. But in the release > consistency model, "rl + aq" is not legal and has no guarantee at all. > > So sc.aqrl should be clarified in spec, but I only found "lr.aq + > sc.aqrl" description, see the patch commit log. The spec doesn't try to enumerate every possible synchronization instruction sequence. That's why the RVWMO rules are in place. > Could we treat sc.aqrl as a whole in ISA? Because in micro-arch, we > must separate it into pieces for implementation. > > That is what the RVWMO should give out. What exactly would you like the spec to say about this? RVWMO and the RISC-V spec in general describe the required architecturally observable behavior. They're not implementation guides. Generally speaking, I would expect splitting an sc.aqrl into a ".rl; sc; .aq" pattern to be OK. That wouldn't introduce new observable behaviors compared to treating the sc.aqrl as a single indivisible operation, would it? Dan >> 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 > If the herd7 model has defined that, I think it should be legal. Good catch. > > >> >> , therefore the ordering between A and B is GMO and GMO should be >> respected by all harts. >> >> Regards, >> Boqun >> >>>> >>>> Dan >>>> >>>>> Regards, >>>>> Boqun >>>>> >>>>>> Andrea >>>>>> >>>>>> >>>>> [...] >>> >>> >>> >>> -- >>> Best Regards >>> Guo Ren >>> >>> ML: https://lore.kernel.org/linux-csky/ > > >