RFR: JDK-8155730: HeapInfoDCmd should get Heap_lock

[Yasumasa Suenaga]

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