RFR: 8265753: Remove manual JavaThread transitions to blocked [v4]

[Robbin Ehn]

On Thu, 20 May 2021 05:14:43 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Robbin Ehn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
>> 
>>  - Review fixes
>>  - Merge branch 'master' into 8265753
>>  - Fixes for Dan
>>  - Merge branch 'master' into 8265753
>>  - Removed manual transitions
>
> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 236:
> 
>> 234: 
>> 235: template <typename PRE_PROC>
>> 236: class ThreadBlockInVMPreprocess : public ThreadStateTransition {
> 
> Can we add a comment before this template definition please:
> 
> // Perform a transition to _thread_blocked and take a call-back to be executed before 
> // SafepointMechanism::process_if_requested when returning to the VM. This allows us
> // to perform an "undo" action if we might block processing a safepoint/handshake operation
> // (such as thread suspension).

Fixed

> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 245:
> 
>> 243:     // Once we are blocked vm expects stack to be walkable
>> 244:     thread->frame_anchor()->make_walkable(thread);
>> 245:     thread->set_thread_state(_thread_blocked);
> 
> This is a pre-existing issue. Everywhere we call make_walkable and then call plain set_thread_state (other than on PPC/Aarch64 which do a release_store) we are at risk of the thread_state update being reordered with stores related to making the stack walkable. Potentially allowing the VMThread (or other thread) to walk a stack that is not yet walkable! The original ObjectMonitor::enter code was aware of this:
> `current->frame_anchor()->make_walkable(current);`
> `// Thread must be walkable before it is blocked.`
> `// Read in reverse order.`
> `OrderAccess::storestore();`
> `for (;;) {`
> `  current->set_thread_state(_thread_blocked);`

Good catch, fixed.

> src/hotspot/share/runtime/objectMonitor.hpp line 309:
> 
>> 307:    protected:
>> 308:     ObjectMonitor* _om;
>> 309:     bool _exited;
> 
> For consistency with raw monitor code this would be _om_exited

Fixed

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

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