RFR: JDK-8317661: [REDO] store/load order not preserved when handling memory pool due to weakly ordered memory architecture of aarch64

[David Holmes]

On Thu, 19 Oct 2023 14:06:38 GMT, Damon Fenacci <dfenacci at openjdk.org> wrote:

>> Sorry I'm still missing part of this (and I know we discussed it a few weeks ago). What is the variable(s) that is written before `_next_segment` that we need to see updated when we call `_codeHeap->capacity()`?
>
> @dholmes-ora is there possibly a way to restrict the update that we need to see to that `_high` variable only?

So ... as we discussed offline a few weeks ago, it is very hard to clearly express the ordering constraints in this code as the two variables are so far apart in the code. So to reiterate, on the writer side we have:

 while (true) {
    cb = (CodeBlob*)heap->allocate(size);  // updates _next_segment
    if (cb != nullptr) break;
    if (!heap->expand_by(CodeCacheExpansionSize)) {  // updates _high

where the writes to `_high` in `expand_by` have to be ordered with the write to `_next_segment` in `allocate` that happen on the next loop iteration.

And on the reader side we have:

size_t used      = used_in_bytes();   // reads _next_segment
size_t committed = _codeHeap->capacity();  // reads _high

where the reads must also be ordered.

I can't help but think that the clearest fix here is to use explicit barriers as discussed before the locking solution was proposed. The problem using `release_store` and `load_acquire` in the bottom level code is that those semantics are only needed in this higher-level code for the `codeHeap`, not for every kind of `heap` the code applies to. 

I wonder if perhaps the following is clearer:

 while (true) {
    cb = (CodeBlob*)heap->allocate(size);
    if (cb != nullptr) break;
    if (!heap->expand_by(CodeCacheExpansionSize)) {
      // ...
    } else {
      OrderAccess::release(); // ensure heap expansion is visible before we allocate on next iteration
    }

coupled with:

size_t used      = used_in_bytes(); 
OrderAccess:acquire();
size_t committed = _codeHeap->capacity(); 


Thoughts?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16143#discussion_r1368253943