RFR: 8276801: gc/stress/CriticalNativeStress.java fails intermittently with Shenandoah [v2]

[Aleksey Shipilev]

On Tue, 9 Nov 2021 17:43:43 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahCodeRoots.cpp line 119:
>> 
>>> 117: 
>>> 118: void ShenandoahCodeRoots::unregister_nmethod(nmethod* nm) {
>>> 119:   assert(CodeCache_lock->owned_by_self(), "Must have CodeCache_lock held");
>> 
>> I don't understand this change.
>> 
>> This method is called from:
>> 
>> 
>>    // Unregister must be done before the state change
>>   {
>>     MutexLocker ml(SafepointSynchronize::is_at_safepoint() ? NULL : CodeCache_lock,
>>                      Mutex::_no_safepoint_check_flag);
>>     Universe::heap()->unregister_nmethod(this);
>>   }
>> 
>> 
>> ...which would mean we _do not_ hold the `CodeCache_lock` at safepoint? Previous assert captured that fact. New one does not. Shouldn't this assert fail intermittently? Same goes for `register_nmethod`, `flush_nmethod`, probably.
>
> Good catch! I revert the assertion in ShenandoahCodeRoots::unregister_nmethod().
> 
> Above code is racy, because there can be a drive-by safepoint when the test is performed. However, it is not problem, because Shenandoah (so as ZGC) only tags the nmethod is unregistered. 
> 
> It is not true for flush_nmethod(), where `CodeCache_lock` must be held.

What about `register_nmethod`? I see it is called from `nmethod::nmethod` that itself does `assert_locked_or_safepoint(CodeCache_lock)`. Is that path guaranteed to always hold the `CodeCache_lock`?

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

PR: https://git.openjdk.java.net/jdk/pull/6316