Does transition_and_fence really assume TSO?

[Daniel D. Daugherty]

On 8/2/16 6:25 PM, David Holmes wrote:
> Hi Andrew,
>
> On 3/08/2016 10:04 AM, Andrew Haley wrote:
>> Here's an interesting comment:
>>
>>   static inline void transition_and_fence(JavaThread *thread, 
>> JavaThreadState from, JavaThreadState to) {
>>     assert(thread->thread_state() == from, "coming from wrong thread 
>> state");
>>     assert((from & 1) == 0 && (to & 1) == 0, "odd numbers are 
>> transitions states");
>>     // Change to transition state (assumes total store ordering!  -Urs)
>>     thread->set_thread_state((JavaThreadState)(from + 1));
>>
>> The "assumes total store ordering!" comment is rather alarming. My
>> processor is not TSO.  But as far as I can see, all this really
>> requires is single-variable coherency.  Is that right?
>
> That comment is pre-historic. The code does not assume TSO. There is a 
> direct, or implicit, full fence between the two stores:
>
>     // Make sure new state is seen by VM thread
>     if (os::is_MP()) {
>       if (UseMembar) {
>         // Force a fence between the write above and read below
>         OrderAccess::fence();
>       } else {
>         // Must use this rather than serialization page in particular 
> on Windows
>         InterfaceSupport::serialize_memory(thread);
>       }
>     }
>
> Cheers,
> David
>
>> Andrew.
>>

Filling in the history here. There are three instances of
"assumes total store ordering" in src/share/vm/runtime/interfaceSupport.hpp:

$ sgv src/share/vm/runtime/interfaceSupport.hpp | grep 'assumes total 
store ordering'
1.178
1.86        // Change to transition state (assumes total store 
ordering!  -Urs)
1.168       // Change to transition state (assumes total store 
ordering!  -Urs)
1.139.2.1           // Change to transition state (assumes total store 
ordering!  -Urs)
568 lines
No id keywords (cm7)


$ sp -r1.86 src/share/vm/runtime/interfaceSupport.hpp
src/share/vm/runtime/SCCS/s.interfaceSupport.hpp:

D 1.86 98/06/10 11:10:44 urs 151 150    00005/00004/00290
MRs:
COMMENTS:
optimizations to entry & exit code of runtime routines


$ sp -r1.139.2.1 src/share/vm/runtime/interfaceSupport.hpp
src/share/vm/runtime/SCCS/s.interfaceSupport.hpp:

D 1.139.2.1 02/10/22 00:51:52 huanghui 259 249  00017/00003/00455
MRs:
COMMENTS:
4683006 add transition_from_native(), which also checks for pending 
suspend request


$ sp -r1.168 src/share/vm/runtime/interfaceSupport.hpp
src/share/vm/runtime/SCCS/s.interfaceSupport.hpp:

D 1.168 05/08/29 15:14:41 kbr 290 289   00045/00020/00515
MRs:
COMMENTS:
6304225 hotspot/runtime_syst IE crashes with b44 libjvm.dll


The oldest delta has no bug ID:

D 1.86 98/06/10 11:10:44 urs 151 150    00005/00004/00290
MRs:
COMMENTS:
optimizations to entry & exit code of runtime routines

and here's the relevant part of the diff:

67,68c68,69
<     assert(thread->thread_state() == from, "comming from wrong thread 
state");
<     // Change to transition state
---
 >     assert(thread->thread_state() == from, "coming from wrong thread 
state");
 >     // Change to transition state (assumes total store ordering!  -Urs)


The change from Hui has a nice comment:

D 1.139.2.1 02/10/22 00:51:52 huanghui 259 249  00017/00003/00455
MRs:
COMMENTS:
4683006 add transition_from_native(), which also checks for pending 
suspend request

so we know that when Hui added transition_from_native() the comment
was copied. Here's the relevant diffs:

102a104,115
 >
 >   static inline void transition_from_native(JavaThread *thread, 
JavaThreadState to) {
 >     assert((to & 1) == 0, "odd numbers are transitions states");
 >     assert(thread->thread_state() == _thread_in_native, "coming from 
wrong thread state");
 >     // Change to transition state (assumes total store ordering!  -Urs)
 >     thread->set_thread_state(_thread_in_native_trans);
 >     if (os::is_MP()) atomic::membar();
 >     if (SafepointSynchronize::do_call_back() || 
thread->is_external_suspend()) {
 >       JavaThread::check_safepoint_and_suspend(thread);
 >     }
 >     thread->set_thread_state(to);
 >   }


The change from Ken has a bug ID:

D 1.168 05/08/29 15:14:41 kbr 290 289   00045/00020/00515
MRs:
COMMENTS:
6304225 hotspot/runtime_syst IE crashes with b44 libjvm.dll

Here's the relevant diffs:

 >   // transition_and_fence must be used on any thread state transition
 >   // where there might not be a Java call stub on the stack, in
 >   // particular on Windows where the Structured Exception Handler is
 >   // set up in the call stub. os::write_memory_serialize_page() can
 >   // fault and we can't recover from it on Windows without a SEH in
 >   // place.
 >   static inline void transition_and_fence(JavaThread *thread, 
JavaThreadState from, JavaThreadState to) {
 >     assert(thread->thread_state() == from, "coming from wrong thread 
state");
 >     assert((from & 1) == 0 && (to & 1) == 0, "odd numbers are 
transitions states");
 >     // Change to transition state (assumes total store ordering!  -Urs)
 >     thread->set_thread_state((JavaThreadState)(from + 1));
 >
 >     // Make sure new state is seen by VM thread
 >     if (os::is_MP()) {
 >       // Must use this rather than serialization page in particular 
on Windows
 >       InterfaceSupport::serialize_memory(thread);
 >     }
 >
 >     if (SafepointSynchronize::do_call_back()) {
 >       SafepointSynchronize::block(thread);
 >     }
 >     thread->set_thread_state(to);
 >
 > CHECK_UNHANDLED_OOPS_ONLY(thread->clear_unhandled_oops();)
 >   }


Filed the following bug:

JDK-8166927 interfaceSupport.hpp has ancient comments about TSO
https://bugs.openjdk.java.net/browse/JDK-8166927

Dan