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?
Dan
> Regards,
> Boqun
>
>> Andrea
>>
>>
> [...]
