RFR(s): 8218147: make_walkable asserts on multiple calls

[Robbin Ehn]

Hi David,

*truncated*

On 4/16/19 9:27 AM, David Holmes wrote:
>> If we want to keep the out-of-line double checking, this is an alternative:
>> diff -r 51211b2d6514 src/hotspot/share/runtime/thread.hpp
>> --- a/src/hotspot/share/runtime/thread.hpp    Tue Apr 16 08:38:32 2019 +0200
>> +++ b/src/hotspot/share/runtime/thread.hpp    Tue Apr 16 08:44:21 2019 +0200
>> @@ -1417,3 +1417,3 @@
>>     bool is_suspend_after_native() const {
>> -    return (_suspend_flags & (_external_suspend | _deopt_suspend)) != 0;
>> +    return (_suspend_flags & (_external_suspend | _deopt_suspend | 
>> _trace_flag)) != 0;
>>     }
> 

So you prefer this patch?

> Right. Should that use JFR_ONLY?

_trace_flag is always present, so we don't need it.
And I'm not sure how to get that macro into that method in a nice way?

> 
> I was a little concerned that thread->set_trace_flag() may not ensure visibility 
> of the flag update, but then realized it should be covered by the fence:
> 
>   static inline void transition_from_native(JavaThread *thread, JavaThreadState 
> to) {
>      // Change to transition state and ensure it is seen by the VM thread.
>      thread->set_thread_state_fence(_thread_in_native_trans);
> 
> and the comment should say:
> 
> // Change to transition state and ensure it is seen by other thread,
> // and we will see any _suspend_flag changes below.
> 
> However it also seems to me that in JfrThreadSampleClosure::do_sample_thread we 
> need a storeload() barrier after:
> 
>    thread->set_trace_flag();
> 
> to ensure its not reordered with the reads of thread->thread_state() ?

Setting/clearing suspend flags is always done with Atomic::cmpxchg, since there 
can be multiple threads manipulating the bit pattern.
I can add a comment about it.

Thanks, Robbin

> 
> Thanks,
> David
> -----
> 
> 
>> So we are missing checking that bit completely in this transition code.
>>
>> Thanks, Robbin
>>
>>
>>> All you are doing is changing the timing of the race between the thread 
>>> re-entering the VM/Java and the request for a suspend or safepoint.
>>>
>>> If there is a race between the sampler logic acting on the thread, and the 
>>> thread acting on itself then that race has to be precluded somehow.
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> (which have the JFR native trans suspend check)
>>>> And it passes t1-5.
>>>>
>>>> Code:
>>>> http://cr.openjdk.java.net/~rehn/8218147/v3/webrev/
>>>> Issue:
>>>> https://bugs.openjdk.java.net/browse/JDK-8218147
>>>>
>>>> Thanks, Robbin
>>>>
>>>> On 4/5/19 5:43 PM, Robbin Ehn wrote:
>>>>> Hi Dean,
>>>>>
>>>>> Sorry, I missed this mail.
>>>>> Yes we can do that.
>>>>> Ignore my other mail, I'll update.
>>>>>
>>>>> Thanks, Robbin
>>>>>
>>>>>
>>>>> dean.long at oracle.com skrev: (5 april 2019 09:22:24 CEST)
>>>>>> On 4/4/19 5:16 PM, dean.long at oracle.com wrote:
>>>>>>>
>>>>>>>>>
>>>>>>>>> If it's already set, should we check that _last_Java_pc matches the
>>>>>>
>>>>>>>>> new value?
>>>>>>>>
>>>>>>>> We manually set the pc in several places, so if it's set, it's not
>>>>>>>> certain that
>>>>>>>> it should be the same as in last sp.
>>>>>>>> I can't distinguish between the cases.
>>>>>>>>
>>>>>>>
>>>>>>> If we get pc from sp[-1] then it should match, but you're right, we
>>>>>>> sometimes get pc from somewhere else.
>>>>>>
>>>>>> How about if we combine the !walkable check and the
>>>>>> capture_last_Java_pc() logic into a single method?
>>>>>> Then we can do something like:
>>>>>>
>>>>>>      if (!walkable()) {
>>>>>>          address pc = (address)_last_Java_sp[-1];
>>>>>>          address a = Atomic::cmpxchg(pc, &_last_Java_pc, NULL);
>>>>>>          assert(a == NULL || a == pc, "unexpected PC %p", a);
>>>>>>      }
>>>>>>
>>>>>> dl