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

[Zhengyu Gu]

On Tue, 9 Nov 2021 16:29:43 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Zhengyu Gu has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Revert assertion for unregister_nmethod
>>  - Fix comment as Aleksey suggested
>
> 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.

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

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