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

[David Holmes]

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.

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