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

[Damon Fenacci]

On Tue, 19 Sep 2023 15:09:31 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> src/hotspot/share/services/memoryPool.cpp line 182:
>> 
>>> 180: MemoryUsage CodeHeapPool::get_memory_usage() {
>>> 181:   OrderAccess::loadload();
>>> 182:   size_t used = used_in_bytes();
>> 
>> This doesn't look quite right. A `loadload` controls the ordering between two accesses. If you want to make sure that you don't see an old version of `committed` with a new version of `used` then the `loadload` must be _between_ the two loads.
>
> Also, for clarity, I'd make `used` volatile, and access it with `Atomic::release_store()` and `load_acquire()`. That should keep everything straight, assuming there aren't any more ordering failures.

> This doesn't look quite right. A `loadload` controls the ordering between two accesses. If you want to make sure that you don't see an old version of `committed` with a new version of `used` then the `loadload` must be _between_ the two loads.

@theRealAph thanks for your comments. You're right: I've just moved it down one line.

> Also, for clarity, I'd make `used` volatile, and access it with `Atomic::release_store()` and `load_acquire()`. That should keep everything straight, assuming there aren't any more ordering failures.

I thought about doing so (@dholmes-ora's suggestion) but we realised that `release_store` would probably need to be put in `CodeCache::allocate` whereas `load_acquire` probably in `CodeHeapPool::get_memory_usage` but then we’d need a reference to be shared between the 2 places and that wouldn't make it very clean (basically there would be "nowhere to put the acquire/release at the low-level").

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15819#discussion_r1331062763