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

[Y. Srinivas Ramakrishna]

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

>> A recent change to avoid checking the log level under the lock inadvertently removed the lock from an operation that needs it.
>
> 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.

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

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