RFR: 8288904: Incorrect memory ordering in UL [v2]

[Erik Österlund]

On Thu, 23 Jun 2022 00:25:21 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> @pchilano, it seems that it is true that a control dependency establishes that writes are not moved above the read of a control branch (as we do not know which, if any, branch is taken before the read is done). read-on-read allows for moving it up however.
>> 
>> For example:
>> 
>> 
>> // OK
>> x = true;
>> while(x == true) { }
>> y = a[0];
>> ~>
>> x = true;
>> y = a[0];
>> while(x == true) {}
>> // NOT OK
>> x = true;
>> while(x == true) { }
>> a[0] = 5;
>> ~>
>> x = true;
>> a[0] = 5;
>> while(x == true) {}
>> 
>> 
>> Source:
>> 
>> https://urldefense.com/v3/__https://www.cl.cam.ac.uk/*pes20/ppc-supplemental/test7.pdf__;fg!!ACWV5N9M2RV99hQ!NCWRDf1vDwPm4fPLuR7h2rfx0zESKwH5pKSsQUW7EXHE7B3Nw4diNRNS9UhaHsrCB1lSR5mTUuV-aNo2XLU4jn7RC-0nfdRWnw$  section 4.2 and 4.4
>> 
>> I believe that that means that this barrier is unnecessary, but it's good manners to do the `Atomic::load`.
>> 
>> Nice catch :-).
>
> @jdksjolen, @fisk thanks for the investigation and linked documents! Mmm there seems to be contradictory definitions then. I also found [1] which as I understand it defines there should be an order between those instructions (sections B2.3.2 and B2.3.3; there is even an intent Note about that). But the fact that we have to look this up and check the fine print is already surprising for me. I thought any cpu would provide that guarantee.
>  
> [1] Arm Architecture Reference Manual: https://urldefense.com/v3/__https://documentation-service.arm.com/static/623b2de33b9f553dde8fd3b0__;!!ACWV5N9M2RV99hQ!NCWRDf1vDwPm4fPLuR7h2rfx0zESKwH5pKSsQUW7EXHE7B3Nw4diNRNS9UhaHsrCB1lSR5mTUuV-aNo2XLU4jn7RC-1hRP_Vcw$ 

Interesting find! So we have 2 ARMv8 memory model documents from ARM, one explicitly saying it may reorder and one explicitly saying it may not.
Since the document saying it won't reorder is more recent, I guess they might have completely changed their minds about this. Has happened before, and seems fine.

While similarly to how we don't exploit data dependency without language level guarantees (we are writing C++, not assembly), I would not exploit control dependencies either, without a C++ level contract saying that's fine, defined in a C++ context. So we should have fences anyway.

However, I would have to assume that ARM would only really be able to update the spec to say this won't reorder, if they knew there are no implementations where it does. So that might at least provide a clue that maybe the reason this code crashed, is due to something different, like maybe the lack of release when allocating new list entries that are injected into the list. So while we might want some fence here anyway, it seems likely it won't stop this code from crashing.

Is it time to wrap the whole thing in a readers writer lock? We already increment and decrement a global variable on the reader side, and writes barely ever happen, but are marked with a lock for writers. It would seemingly fix the problems.

-------------

PR: https://git.openjdk.org/jdk/pull/9225