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

[Damon Fenacci]

On Tue, 24 Oct 2023 08:56:44 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> 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?
>
>> 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
> 
> "visible to an asynchronous observer (e.g. CodeHeapPool::get_memory_usage())"
> 
>> before we allocate on next iteration
>>     }
>> ```
>> 
>> coupled with:
>> 
>> ```
>> size_t used      = used_in_bytes(); 
>> OrderAccess:acquire();
>> size_t committed = _codeHeap->capacity(); 
>> ```
>> 
>> Thoughts?
> 
> As long as it's more thoroughly commented, i guess so.
> 
> A floating release is, however, a pretty bad code smell. The only reason we're aren't solving this problem (IMHO) properly is that the code cache lock isn't re-entrant.

>  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
>     }
> ```

This would definitely make sense.

> A floating release is, however, a pretty bad code smell. The only reason we're aren't solving this problem (IMHO) properly is that the code cache lock isn't re-entrant.

Indeed, it still looks a bit "messy". I was also wondering if there could be a performance penalty by using explicit barriers (although at least `OrderAccess::release()` won't be called very often with this solution).

In any case I've modified the code to make use of explicit barriers following @dholmes-ora suggestion.

Thanks a lot!

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

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