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

[Andrew Haley]

On Wed, 20 Sep 2023 06:27:58 GMT, Damon Fenacci <dfenacci at openjdk.org> wrote:

>> 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").

Firstly, this code looks horribly racy. There is a CodeCache lock, but it seems to be that the memory tracking is accessing data outside the lock. I don't know how that's supposed to work.

It makes no sense to have a memory fence without explicitly saying which accesses it's ordering. In this patch that's now clear on the reader side, but not on the writer side. For the future maintainer of the code, we need to be able to see that.

What do you mean by "we’d need a reference to be shared between the 2 places "?

The use of naked storestore fences is a black-belt-ninja thing, and almost certainly not what you want here. See https://hboehm.info/c++mm/no_write_fences.html.

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

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