by Daniel Lustig

[permalink] [raw]

Subject: Re: [PATCH V4 5/5] riscv: atomic: Optimize LRSC-pairs atomic ops with .aqrl annotation

On 7/13/2022 7:47 PM, Guo Ren wrote:
> On Thu, Jul 7, 2022 at 8:04 AM Boqun Feng <[email protected]> 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 <[email protected]> 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/[email protected]/raw
>>>>>>
>>>>>> "Thoughts" on this regard have _changed_. Please compare that quote
>>>>>> with, e.g.
>>>>>>
>>>>>> https://lore.kernel.org/linux-riscv/[email protected]/
>>>>>>
>>>>>> 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/
>
>
>

2022-08-09 07:09:32

by Guo Ren

[permalink] [raw]

Subject: Re: [PATCH V4 5/5] riscv: atomic: Optimize LRSC-pairs atomic ops with .aqrl annotation

On Thu, Jul 14, 2022 at 9:06 PM Dan Lustig <[email protected]> wrote:
>
> On 7/13/2022 7:47 PM, Guo Ren wrote:
> > On Thu, Jul 7, 2022 at 8:04 AM Boqun Feng <[email protected]> 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 <[email protected]> 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/[email protected]/raw
> >>>>>>
> >>>>>> "Thoughts" on this regard have _changed_. Please compare that quote
> >>>>>> with, e.g.
> >>>>>>
> >>>>>> https://lore.kernel.org/linux-riscv/[email protected]/
> >>>>>>
> >>>>>> 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.
Okay, I just want to confirm "lr + sc.aqrl" is correct here.

>
> > 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?
Yes, I think the below modification is correct, and it could improve
the performance in the fast path. Adding .aq annotation during the
false loop won't cause side effects. right?
"0: lr.d %[p], %[c]\n"
" beq %[p], %[u], 1f\n"
" add %[rc], %[p], %[a]\n"
- " sc.d.rl %[rc], %[rc], %[c]\n"
+ " sc.d.aqrl %[rc], %[rc], %[c]\n"
" bnez %[rc], 0b\n"
- " fence rw, rw\n"

>
> 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/
> >
> >
> >

--
Best Regards
Guo Ren