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

[Yasumasa Suenaga]

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