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

[David Holmes]

On 16/04/2019 5:56 pm, Robbin Ehn wrote:
> 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?

Yes

> 
>> Right. Should that use JFR_ONLY?
> 
> _trace_flag is always present, so we don't need it.

Okay ... not clear what will set it other than JFR ...

> 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;

>>
>> 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