PING: RFR: JDK-8155730: HeapInfoDCmd should get Heap_lock

[David Holmes]

On 15/06/2016 12:14 AM, Erik Helin wrote:
> On 2016-06-14, David Holmes wrote:
>> Hi Erik,
>>
>> On 13/06/2016 11:23 PM, Erik Helin wrote:
>>> On 2016-06-02, Yasumasa Suenaga wrote:
>>>> PING: Could you review it?
>>>>
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8155730/webrev.02/
>>>
>>> This will not work - what if the VM crashes while holding the HeapLock?
>>
>> I assume you are referring to acquiring the HeapLock in vmError.cpp. But
>> that code is not used for actual error reporting only the VMInfo Dcmd.
>
> Ah alright, I didn't get that that (should that code be moved somewhere
> else?).

That debate was held when the code was put there. :) The Dcmd produces 
the same general info as is in the error report, hence all that code is 
co-located.

Thanks,
David

> Thanks,
> Erik
>
>> Thanks,
>> David
>>
>>> Erik
>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2016/05/17 19:19, Yasumasa Suenaga wrote:
>>>>> PING: Could you review it?
>>>>> I need review from GC folks.
>>>>>
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8155730/webrev.02/
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2016/05/10 22:25, Yasumasa Suenaga wrote:
>>>>>> On 2016/05/10 16:48, David Holmes wrote:
>>>>>>> On 9/05/2016 10:34 PM, Yasumasa Suenaga wrote:
>>>>>>>> Hi David,
>>>>>>>>
>>>>>>>>> Taking locks during VMError handling is problematic - we're executing
>>>>>>>> >from a signal handler and the VM is crashing. Much of what we do from
>>>>>>>>> VMError is "best effort".
>>>>>>>>
>>>>>>>> I will remove MutexLocker from VMError::report().
>>>>>>>> But I added assert_locked_or_safepoint(Heap_lock) in each CollectedHeap.
>>>>>>>> If we remove Heap_lock from VMError::report(), fastdebug HotSpot is failed
>>>>>>>> when it prints heap info to hs_err log.
>>>>>>>> Should I remove assert_locked_or_safepoint()?
>>>>>>>
>>>>>>> Yes I think you need to do this.
>>>>>>
>>>>>> I removed them in new webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8155730/webrev.02/
>>>>>>
>>>>>>>> VMError::print_vm_info() calls from VMInfoDCmd::execute() only.
>>>>>>>> So I think we should take Heap_lock at this function.
>>>>>>>> Or should we take Heap_lock at VMInfoDCmd::execute() ?
>>>>>>>
>>>>>>> If this is called from a DCmd only then I think the lock can be taken in print_vm_info().
>>>>>>
>>>>>> I grep'ed HotSpot source, print_vm_info() is called from DCmd only.
>>>>>>
>>>>>>> Need GC folk to add their weight here.
>>>>>>
>>>>>> I'm waiting reviewer from GC folks :-)
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2016/05/09 21:26, David Holmes wrote:
>>>>>>>>> On 9/05/2016 10:20 PM, Yasumasa Suenaga wrote:
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> I've sent review request for JDK-8155730.
>>>>>>>>>> However, I found caller of Universe::heap()->print_*() in vmError.cpp .
>>>>>>>>>> Thus I added MutexLocker(Ex) for Heap_lock to VMError::report() and
>>>>>>>>>> VMError::print_vm_info().
>>>>>>>>>
>>>>>>>>> Taking locks during VMError handling is problematic - we're executing
>>>>>>>> >from a signal handler and the VM is crashing. Much of what we do from
>>>>>>>>> VMError is "best effort".
>>>>>>>>>
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>> Could you review again?
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8155730/webrev.01/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> This needs to be reviewed by the GC folk not runtime. Please direct
>>>>>>>>>>> followups to hotspot-gc-dev.
>>>>>>>>>>
>>>>>>>>>> Thanks, David!
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2016/05/06 13:11, David Holmes wrote:
>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>
>>>>>>>>>>> This needs to be reviewed by the GC folk not runtime. Please direct
>>>>>>>>>>> followups to hotspot-gc-dev.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> David
>>>>>>>>>>>
>>>>>>>>>>> On 30/04/2016 12:05 AM, Yasumasa Suenaga wrote:
>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>
>>>>>>>>>>>> This review request is related to [1].
>>>>>>>>>>>>
>>>>>>>>>>>> HeapInfoDCmd calls CollectedHeap::print_on().
>>>>>>>>>>>> However, its call does not get Heap_lock.
>>>>>>>>>>>>
>>>>>>>>>>>> Heap_lock should be gotten from all heap operations.
>>>>>>>>>>>>
>>>>>>>>>>>> I uploaded webrev for this issue.
>>>>>>>>>>>> Could you review it?
>>>>>>>>>>>>
>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8155730/webrev.00/
>>>>>>>>>>>>
>>>>>>>>>>>> I cannot access JPRT.
>>>>>>>>>>>> So I need a sponsor.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> [1]
>>>>>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-April/019257.html
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>