RFR: 8341554: Shenandoah: Missing heap lock when updating usage for soft ref policy

[William Kemper]

On Fri, 4 Oct 2024 20:48:35 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 186:
>> 
>>> 184:         ShenandoahHeapLocker locker(heap->lock());
>>> 185:         heap->update_capacity_and_used_at_gc();
>>> 186:       }
>> 
>> If free set logging is enabled this does a lock/unlock for logging and then another lock/unlock for updating capacity.
>> 
>> But I guess it's unavoidable, and it likely won't be the case that we have free set logging enabled in performance critical production situations, so not worth wasting too much sleep over.
>> 
>> Reviewed!
>
> @earthling-amzn : I'd suggest leaving a comment either in the ticket or in the PR (or here in the code?) stating the race that we are vulnerable to if we don't hold the lock, namely the potential skew between used & capacity, and why it's important not to have that skew for consumers of these fields. I imagine that we can demonstrate the issue with a targeted regression test.

Yeah, I think coupling the lock with the logging and `update_capacity_and_used` is what led us into trouble to begin with.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21362#discussion_r1788371332