[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR (M) 8005012: Add WB APIs to better support NMT testing
From: David Holmes <david.holmes () oracle ! com>
Date: 2013-01-25 5:47:17
Message-ID: 51021C65.2070005 () oracle ! com
[Download RAW message or body]
Hi Christian,
On 25/01/2013 4:25 AM, Christian Törnqvist wrote:
> Hi David,
>
> My suggestion is that I open a bug to clean up the snapshot->wait() thing in both \
> MemTrackWorker::run() and MemTracker::wbtest_wait_for_data_merge() and do this as \
> separate task since this would also change the way NMT works today.
> As for this change I think there are two options, I can either just replace the \
> snapshot->wait() call in wbtest_wait_for_data_merge() with os::sleep(), this would \
> make it different from MemTrackWorker::run() but most probably wouldn't break \
> anything. Or I could leave it as is and do the cleanup in both methods at the same \
> time.
The cleanup can wait.
Thanks,
David
> Thanks,
> Christian
>
> -----Original Message-----
> From: Christian Törnqvist
> Sent: den 21 januari 2013 14:03
> To: David Holmes; Zhengyu Gu
> Cc: hotspot-runtime-dev@openjdk.java.net
> Subject: RE: RFR (M) 8005012: Add WB APIs to better support NMT testing
>
> Hi David,
>
> > Then add a comment here:
> >
> > 577 bool MemTracker::wbtest_wait_for_data_merge() {
> > + // NMT can't be shutdown while we hold this lock
> > 578 MutexLockerEx lock(_query_lock, true);
>
> I've added the comment
>
> > but I don't see where any notify/notifyAll is now taking place ???
>
> There is no notify/notifyAll on the snapshot _lock, the only time it's used is by \
> MemTrackWorker::run() and MemTracker::wbtest_wait_for_data_merge() which both call \
> wait(1000) on it and could be replace by a call to os::sleep() if waiting 1000ms is \
> the only thing we're after here, Zhengyu might know? Not sure if it's in the scope \
> of this change to correct this though?
> Thanks,
> Christian
>
> -----Original Message-----
> From: David Holmes
> Sent: den 15 januari 2013 02:05
> To: Zhengyu Gu
> Cc: Christian Törnqvist; hotspot-runtime-dev@openjdk.java.net
> Subject: Re: RFR (M) 8005012: Add WB APIs to better support NMT testing
>
> On 15/01/2013 12:56 AM, Zhengyu Gu wrote:
> > On 1/14/2013 9:27 AM, Christian Törnqvist wrote:
> > > > Yes, have notify_all in destructor is a really bad idea, but you do
> > > > need notify_all before delete memSnapshot, since
> > > > MemTracker::wbtest_wait_for_data_merge() can still wait on snapshot ...
> > > But the only place where we delete snapshot is in
> > > MemTracker::final_shutdown() which acquires query_lock before
> > > deleting snapshot so wbtest_wait_for_data_merge() will not be able to
> > > wait on snapshot at this point.
> > >
> > You are right. Look good to me.
>
> Then add a comment here:
>
> 577 bool MemTracker::wbtest_wait_for_data_merge() {
> + // NMT can't be shutdown while we hold this lock
> 578 MutexLockerEx lock(_query_lock, true);
>
> but I don't see where any notify/notifyAll is now taking place ???
>
> David
>
> > Thanks,
> >
> > -Zhengyu
> >
> > > Thanks,
> > > Christian
> > >
> > > -----Original Message-----
> > > From: Zhengyu Gu
> > > Sent: den 11 januari 2013 15:16
> > > To: David Holmes
> > > Cc: Christian Törnqvist; hotspot-runtime-dev@openjdk.java.net
> > > Subject: Re: RFR (M) 8005012: Add WB APIs to better support NMT
> > > testing
> > >
> > >
> > > Please see comments inline.
> > >
> > > On 1/11/2013 1:48 AM, David Holmes wrote:
> > > > On 10/01/2013 8:52 PM, Christian Törnqvist wrote:
> > > > > Hi David,
> > > > >
> > > > > > This method blocks while holding the query_lock - is that intended?
> > > > > Yes, this will prevent NMT from shutting down and freeing the data
> > > > > structures we're using.
> > > > Ok. Of course this screams deadlock potential to me :)
> > > >
> > > No, I don't think that there is deadlock potential. _query_lock is
> > > used to serialize NMT queries, there is no NMT related lock can be
> > > acquired before it.
> > >
> > > > > > Aside: the MemSnapshot::wait method is troubling me. What are you
> > > > > > waiting upon ie what state condition? The notification is done in
> > > > > > the destructor but how can the destructor be called if someone is
> > > > > > calling
> > > > > > wait() upon the object's lock ??? It means you are destroying an
> > > > > > object that is still in use, including the lock that is being
> > > > > > waited upon!!!
> > > Yes, have notify_all in destructor is a really bad idea, but you do
> > > need notify_all before delete memSnapshot, since
> > > MemTracker::wbtest_wait_for_data_merge() can still wait on snapshot ...
> > > How about in MemTracker::final_shutdown()
> > >
> > > _snapshot->notify_all_waiters();
> > > delete _snapshot;
> > >
> > > Thanks,
> > >
> > > -Zhengyu
> > >
> > > > > This was obviously incorrect, thanks for catching this. I've
> > > > > removed it and done some small other cleanups, a new webrev can be
> > > > > found at http://cr.openjdk.java.net/~ehelin/8005012/webrev.01/
> > > > I don't see memSnapshot in the new webrev - was this all new stuff
> > > > that is now gone?
> > > >
> > > > David
> > > >
> > > >
> > > > > Thanks,
> > > > > Christian
> > > > >
> > > > > -----Original Message-----
> > > > > From: David Holmes
> > > > > Sent: den 19 december 2012 00:21
> > > > > To: Christian Törnqvist
> > > > > Cc: hotspot-runtime-dev@openjdk.java.net; Zhengyu Gu
> > > > > Subject: Re: RFR (M) 8005012: Add WB APIs to better support NMT
> > > > > testing
> > > > >
> > > > > On 19/12/2012 12:10 AM, Christian Törnqvist wrote:
> > > > > > Hi everyone,
> > > > > >
> > > > > > This change is about adding three new WB APIs to better support
> > > > > > NMT testing. The changes are:
> > > > > >
> > > > > > ˇA Test memory type, used by WB API's NMTAllocTest and
> > > > > > NMTFreeTestMemory
> > > > > >
> > > > > > ˇAdded a WaitForDataMerge WB API to be able to block until the
> > > > > > current generation has been merged and is visible, most of this
> > > > > > work was done by Zhengyu Gu.
> > > > > This method blocks while holding the query_lock - is that intended?
> > > > >
> > > > > Aside: the MemSnapshot::wait method is troubling me. What are you
> > > > > waiting upon ie what state condition? The notification is done in
> > > > > the destructor but how can the destructor be called if someone is
> > > > > calling
> > > > > wait() upon the object's lock ??? It means you are destroying an
> > > > > object that is still in use, including the lock that is being
> > > > > waited upon!!!
> > > > >
> > > > > David
> > > > > -----
> > > > >
> > > > > > Webrev can be found at:
> > > > > >
> > > > > > http://cr.openjdk.java.net/~ehelin/8005012/webrev.00/
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > Christian
> > > > > >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic