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

[Stefan Karlsson]

On Wed, 31 May 2023 11:08:06 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> [JDK-8307106](https://bugs.openjdk.org/browse/JDK-8307106) introduced the ability to walk the created CLDs list in the CLDG without a lock. This was primarily introduced to allow lockless concurrent CLD roots scanning for young collections in generational ZGC. However because the CLD _next node pointer is shared between the two list this can lead to a concurrent iteration of the created CLDs missing list entries. 
>> 
>> This change introduces a second _unloading_next node pointer which is used for the unloading CLDs list. The set_next is now maintains the invariant that it only ever unlinks is_unloading() CLDs and maintains a consistent view of the tail list for anyone reading the list concurrently.  
>> 
>> Testing: tier1-3 and tier1-7 with Generational ZGC
>
> 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

I think the patch looks good. I've added a suggestion for a rewording of the comment about the two list. Feel free to skip it if you liked your comment better.

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();

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

Marked as reviewed by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14241#pullrequestreview-1453025487
PR Review Comment: https://git.openjdk.org/jdk/pull/14241#discussion_r1211604141