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

[Damon Fenacci]

On Wed, 20 Sep 2023 12:26:17 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> 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.
>
> Aha! I think it's a bug. Here:
> 
> 
>   // Track memory usage statistic after releasing CodeCache_lock
>   MemoryService::track_code_cache_memory_usage();
> 
> 
> calls
> 
> 
>   // Track the peak memory usage
>   pool->record_peak_memory_usage();
> 
> 
> calls
> 
> 
>   // Caller in JDK is responsible for synchronization -
>   // acquire the lock for this memory pool before calling VM
>   MemoryUsage usage = get_memory_usage();
> 
> 
> Oops. I can't see the lock being acquired.

>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

Acquiring a CodeCache lock when getting `used` and `committed` here
https://github.com/openjdk/jdk/blob/12de9b0225363377e9a76729b11698221d4f29f2/src/hotspot/share/services/memoryPool.cpp#L181-L182
was actually the solution in the first commit https://github.com/openjdk/jdk/pull/15819/commits/1060bb01f7ad0ca47c38eb0f68b7f2aab6562ac3 but then, asking about the expensiveness of a lock vs storestore/loadload barriers, and the fact that the origin of the issue was actually the order in which store and load actually happen, made me think barriers would be preferable, thing that I now very much doubt.
You are suggesting that it would make more sense (and be cleaner) to acquire a CodeCache lock instead of using barriers, right?

> ```
>   // Caller in JDK is responsible for synchronization -
>   // acquire the lock for this memory pool before calling VM
>   MemoryUsage usage = get_memory_usage();
> ```
> 
> Oops. I can't see the lock being acquired.

I'm wondering if there are more places where this would be needed...


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

What I meant is mainly that in this specific case the _storing_ and _loading_ happens in very different places in the code.

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

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