RFR: 8308387: CLD created and unloading list sharing _next node pointer leads to concurrent YC missing CLD roots [v2]

[Axel Boldt-Christmas]

On Wed, 31 May 2023 12:04:29 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Axel Boldt-Christmas has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Stefank feedback: introduce unlink_next, rework do_unloading loop
>
> src/hotspot/share/classfile/classLoaderData.hpp line 167:
> 
>> 165:   // ClassLoaderDataGraph::purge(), so some syncronization is required between
>> 166:   // the two to ensure that no unlinked CLD remains in the system leading
>> 167:   // to a use after free.
> 
> I talked to Axel about the wording around what the two list contained, and how there's a slight risk that someone who's not up-to-speed in this area might misinterpret parts of the comment.
> 
> What do you think about something along the lines of this suggestion?:
> 
> Suggestion:
> 
>   //
>   // The ClassLoaderDataGraph maintains two lists to keep track of CLDs.
>   //
>   // The first list [_head, _next] is where new CLDs are registered. The CLDs
>   // are only inserted at the _head, and the _next pointers are never rewritten.
>   // This allows GCs to concurrently walk the list while the CLDs are being
>   // concurrently unlinked.
>   //
>   // The second list [_unloading_head, _unloading_next] is where dead CLDs get
>   // moved to during class unloading. See: ClassLoaderDataGraph::do_unloading().
>   // This list is never modified while other threads are iterating over it. 
>   //
>   // After all dead CLDs have been moved to the unloading list, there's a
>   // synchronziation point (handshake) to ensure that all threads reading these
>   // CLDs finish their work. This ensures that we don't have a user-after-free
>   // when we later delete the CLDs. 
>   //
>   // And finally, when no threads are using the unloading CLDs anymore, we
>   // remove them from the class unloading list and delete them. See:
>   // ClassLoaderDataGraph::purge();

Your comment seems more structured, I will use it. Will only update the `and the _next pointers are never rewritten.` part to reflect the fact that `unlink_next()` will update it by removing one is_unloading CLD.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14241#discussion_r1211640634