RFR: 8335217: Fix memory ordering in ClassLoaderData::ChunkedHandleList [v2]

[Zhengyu Gu]

On Thu, 27 Jun 2024 13:40:16 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

> > > Did you see that ChunkedHandleList::_size is also updated concurrently? If there's truly a concurrency order that this fixes, then maybe we should at least use Atomic::load when we're reading the _size?
> > 
> > 
> > You mean `ChunkedHandleList::Chunk::_size`, right?
> 
> Oops. Right.
> 
> > It is updated via `Atomic::release_store()` [here](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/classfile/classLoaderData.cpp#L199) and read via `Atomic::load_acquire()` [here](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/classfile/classLoaderData.cpp#L223).
> > And now, I believe it also needs `Atomic::load_acquire()` for [this read](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/classfile/classLoaderData.cpp#L225)
> 
> I think the first load_acquire covers the rest of the memory further down the list. So, I don't think it is required there.

This is something I am not sure about: are you saying that this `load_acquire` has global effect? because `release_store()`s are on different memory locations.

> 
> > as well and I agree that should use `Atomic::load()` to make it more explicit.
> 
> I don't think it is needed in ChunkedHandleList::add because it is modified under a lock. ChunkedHandleList::count should be using `Atomic::load`.

Okay, I will revert this.

> 
> FWIW, I find it odd that we would need a load_acquire in the destructor. We shouldn't have concurrently modifying threads if the CLD is about to be deleted.

I don't disagree that there are many synchronization points between `ClassLoaderData::ChunkedHandleList::add()` and unload CLD, but it is not obvious.

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

PR Comment: https://git.openjdk.org/jdk/pull/19919#issuecomment-2194842092