RFR: 8296101: nmethod::is_unloading result unstable with concurrent unloading

[Dean Long]

On Tue, 1 Nov 2022 10:39:56 GMT, Kim Barrett <kbarrett 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.
>
> src/hotspot/share/code/nmethod.cpp line 1673:
> 
>> 1671:   // determined. This can't recurse as the second time we call the state is
>> 1672:   // guaranteed to be cached for the current unloading cycle.
>> 1673:   return is_unloading();
> 
> FWIW, it seems like you could remove any question about recursion by doing
> something like this:
> 
> uint8_t found_state = Atomic::cmpxchg(&_is_unloading_state, state, new_state, memory_order_relaxed);
> uint8_t updated_state = (found_state == state) ? new_state : found_state;
> return IsUnloadingState::is_unloading(updated_state);
> 
> Is that clearer (esp. with corresponding commentary)?  Maybe.

I think avoiding recursion is clearer.  How about:


if (found_state == state) {
  // First to change state, we win
  return state_is_unloading;
} else {
  // State already set, so use it
  return IsUnloadingState::is_unloading(found_state);
}

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

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