RFR: 8342444: Shenandoah: Uncommit regions from a separate, STS aware thread

[William Kemper]

On Mon, 11 Nov 2024 18:26:29 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 555:
>> 
>>> 553:     while (region != nullptr) {
>>> 554:       {
>>> 555:         ShenandoahHeapLocker locker(heap->lock());
>> 
>> Was it a bug that previous version of this code did not acquire the heap lock?
>> 
>> Is the lock required for the entirety of time that we are clearing the bitmap?  Or is it just required to get a trustworthy check on is_bitmap_slice_committed()?
>
> After reading more of this PR, I believe we need the heap lock to get a reliable signal of bitmap_slice_committed().  But I believe we do not need the heap lock for ctx->clear_bitmap(region) so would prefer to move that outside the lock, unless I am misunderstanding.

Hmm, I'm not sure we can do that. Prior to this change, the control thread performed both clearing the bitmap and uncommitting the region's bitmap, so they could never happen concurrently. With this change, a separate thread could perform the uncommit. Consider:

1. Control thread takes heap lock, observes that bitmap slice for region A is committed
2. Control thread releases heap lock, begins clearing bitmap (writing zeros to bitmap slice)
3. Uncommit thread takes heap lock, believes it must uncommit region A
4. Uncommit thread uncommits bitmap slice for region A
5. Segfault in Control Thread

I do _believe_ if we had a per region lock, it would be useful here. Holding a lock over the entire heap for this feels like overkill. Or, we could schedule the uncommit so that it does not occur during a GC cycle.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22019#discussion_r1837215468