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 13:27:33 GMT, Damon Fenacci <dfenacci at openjdk.org> wrote:

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

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

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

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