RFR: 8296101: nmethod::is_unloading result unstable with concurrent unloading [v2]

[Kim Barrett]

On Wed, 2 Nov 2022 10:02:37 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

>> If an nmethod is not called during a concurrent full GC, then after marking has terminated, multiple threads can call is_unloading. If at the same time, the nmethod is made not_entrant, then we run into a source of instability in the is_cold() calculation used when computing is_unloading. There we check if the nmethod is_not_entrant(), which some concurrent observers will think is true, while others think it's false.
>> The current code that sets the is_unloading_state in is_unloading() assumes that the computed state is the same across all observers. However, that is no longer true.
>> 
>> I propose to set the is_unloading_state with a CAS instead of plain store. Then, as is_unloading() is computed before making nmethods not_entrant, we can guarantee that all concurrent readers of is_unloading in this scenario will return false in the current unloading cycle, instead of racingly returning either false or true. One thread wins, and it will say false, and the other threads will compute conflicting results, but end up agreeing after the CAS, that they should all return false.
>> 
>> Tested with mach5 tier1-7. Also tried replacing the is_not_entrant() ingredient in is_cold with os::random() to simulate the instability source. Without my fix RunThese crashes almost immediately, and with my fix it doesn't crash.
>
> Erik Österlund has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Avoid recursion

Recursion removal looks good.

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

Marked as reviewed by kbarrett (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10926