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 14:16:44 GMT, Andrew Haley <aph at openjdk.org> wrote:

>>>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.
>
>> > 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 [1060bb0](https://github.com/openjdk/jdk/commit/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?
> 
> That seems to be the established convention. And I wonder if there are other problems caused by accessing data in this way, outside the CodeCache lock.
> 
>> > ```
>> >   // 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...
> 
> The comment makes it pretty clear that `get_memory_usage()`really needs to hold the lock for this memory pool. I very strongly suspect this is the real cause of your problem.
> 
>> > 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.
> 
> Sure, it's not best practice. Better than an (apparently) randomly-placed fence, though.

I’ve reverted to acquiring the `CodeCache_lock`  while reading `used` and `committed`.
I thought about acquiring it higher up in the call stack but for this problematic case it is not really possible: in our case `get_memory_usage` is called from here https://github.com/openjdk/jdk/blob/12de9b0225363377e9a76729b11698221d4f29f2/src/hotspot/share/services/management.cpp#L592-L603
which is not really related to CodeCache (wrong place to acquire a `CodeCache_lock`) and it gets called from outside the VM.

The other problem I thought might happen is a deadlock due to `get_memory_usage` being called after a `CodeCache_lock` has already been acquired. This does’t seem to be the case (checked by inspecting possible call stacks and running tier 1-5+ tests). Acquiring the lock conditionally (e.g. `CompiledMethod_lock->owned_by_self() ? nullptr : CompiledMethod_lock`, used in for other locks in the code) might guarantee that this doesn't happen but seems to "pollute" the code unnecessarily.

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

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