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

[Robbin Ehn]

Hi David,

>> And I'm not sure how to get that macro into that method in a nice way?
> 
> Define nice ;-)
> 
> return (_suspend_flags & (_external_suspend | _deopt_suspend JFR_ONLY(| 
> _trace_flag))) != 0;

Sure!

I'll re-test and sent out a v4, thanks!

/Robbin

> 
>>>
>>> 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.
> 
> Missed that - thanks.
> 
> David
> -----
> 
>> 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