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

[Erik Helin]

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?).

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