Changeset 36839 in vbox

Timestamp:

Apr 25, 2011 5:29:21 PM (14 years ago)

Author:

vboxsync

Message:

Main/Metrics: Locking revised to prevent lockups on VM shutdown (#5637)

Location:

trunk/src/VBox/Main

Files:

• include/Performance.h (modified) (6 diffs)
• src-server/MachineImpl.cpp (modified) (1 diff)
• src-server/Performance.cpp (modified) (5 diffs)
• src-server/PerformanceImpl.cpp (modified) (5 diffs)

trunk/src/VBox/Main/include/Performance.h

@@ -145 +145 @@
         ~CollectorGuest();
 
+        bool isUnregistered()   { return mUnregistered; };
         bool isEnabled()        { return mEnabled; };
         bool isValid()          { return mValid; };
         void invalidateStats()  { mValid = false; };
+        void unregister()       { mUnregistered = true; };
         int updateStats();
         int enable();
@@ -168 +170 @@
 
     private:
+        bool                 mUnregistered;
         bool                 mEnabled;
         bool                 mValid;
@@ -194 +197 @@
     public:
         CollectorGuestManager() : mVMMStatsProvider(NULL) {};
+        ~CollectorGuestManager() { Assert(mGuests.size() == 0); };
         void registerGuest(CollectorGuest* pGuest);
         void unregisterGuest(CollectorGuest* pGuest);
         CollectorGuest *getVMMStatsProvider() { return mVMMStatsProvider; };
         void preCollect(CollectorHints& hints, uint64_t iTick);
+        void destroyUnregistered();
     private:
         CollectorGuestList mGuests;
@@ -234 +239 @@
     public:
         BaseMetric(CollectorHAL *hal, const char *name, ComPtr<IUnknown> object)
-            : mPeriod(0), mLength(0), mHAL(hal), mName(name), mObject(object), mLastSampleTaken(0), mEnabled(false) {};
+            : mPeriod(0), mLength(0), mHAL(hal), mName(name), mObject(object),
+              mLastSampleTaken(0), mEnabled(false), mUnregistered(false) {};
         virtual ~BaseMetric() {};
 
@@ -249 +255 @@
         void enable()  { mEnabled = true; };
         void disable() { mEnabled = false; };
-
+        void unregister() { mUnregistered = true; };
+
+        bool isUnregistered() { return mUnregistered; };
         bool isEnabled() { return mEnabled; };
         ULONG getPeriod() { return mPeriod; };
@@ -265 +273 @@
         uint64_t         mLastSampleTaken;
         bool             mEnabled;
+        bool             mUnregistered;
     };
 

trunk/src/VBox/Main/src-server/MachineImpl.cpp

@@ -10562 +10562 @@
     {
         mParent->performanceCollector()->unregisterGuest(mCollectorGuest);
-        delete mCollectorGuest;
+        // delete mCollectorGuest; => CollectorGuestManager::destroyUnregistered()
         mCollectorGuest = NULL;
     }
+#if 0
     // Trigger async cleanup tasks, avoid doing things here which are not
     // vital to be done immediately and maybe need more locks. This calls
     // Machine::unregisterMetrics().
     mParent->onMachineUninit(mPeer);
+#else
+    /*
+     * It is safe to call Machine::unregisterMetrics() here because
+     * PerformanceCollector::samplerCallback no longer accesses guest methods
+     * holding the lock.
+     */
+    unregisterMetrics(mParent->performanceCollector(), mPeer);
+#endif
 
     if (aReason == Uninit::Abnormal)

trunk/src/VBox/Main/src-server/Performance.cpp

@@ -110 +110 @@
 
 CollectorGuest::CollectorGuest(Machine *machine, RTPROCESS process) :
-    mEnabled(false), mValid(false), mMachine(machine), mProcess(process),
+    mUnregistered(false), mEnabled(false), mValid(false), mMachine(machine), mProcess(process),
     mCpuUser(0), mCpuKernel(0), mCpuIdle(0),
     mMemTotal(0), mMemFree(0), mMemBalloon(0), mMemShared(0), mMemCache(0), mPageTotal(0),
@@ -202 +202 @@
 void CollectorGuestManager::preCollect(CollectorHints& hints, uint64_t /* iTick */)
 {
+    /*
+     * Since we are running without a lock the value of mVMMStatsProvider
+     * can change at any moment. In the worst case we won't collect any data.
+     */
     CollectorGuestList::iterator it;
 
@@ -211 +215 @@
                     this, __PRETTY_FUNCTION__, *it, (*it)->getProcess(),
                     hints.isGuestStatsCollected((*it)->getProcess())?"y":"n"));
+        if ((*it)->isUnregistered())
+            continue;
         if (  (hints.isHostRamVmmCollected() && *it == mVMMStatsProvider)
             || hints.isGuestStatsCollected((*it)->getProcess()))
@@ -252 +258 @@
     LogAleksey(("{%p} " LOG_FN_FMT ": About to unregister guest=%p provider=%p\n",
                 this, __PRETTY_FUNCTION__, pGuest, mVMMStatsProvider));
-    mGuests.remove(pGuest);
-    LogAleksey(("{%p} " LOG_FN_FMT ": Number of guests after remove is %d\n",
-                this, __PRETTY_FUNCTION__, mGuests.size()));
+    //mGuests.remove(pGuest); => destroyUnregistered()
+    pGuest->unregister();
     if (pGuest == mVMMStatsProvider)
     {
         /* This was our VMM stats provider, it is time to re-elect */
-        if (mGuests.empty())
-        {
-            /* Nobody can provide VMM stats */
-            mVMMStatsProvider = NULL;
-        }
-        else
-        {
-            /* First let's look for a guest already collecting stats */
-            CollectorGuestList::iterator it;
-
-            for (it = mGuests.begin(); it != mGuests.end(); it++)
-                if ((*it)->isEnabled())
-                {
-                    /* Found one, elect it */
-                    mVMMStatsProvider = *it;
-                    LogAleksey(("{%p} " LOG_FN_FMT ": LEAVE new provider=%p\n",
-                                this, __PRETTY_FUNCTION__, mVMMStatsProvider));
-                    return;
-                }
-
-            /* Nobody collects stats, take the first one */
-            mVMMStatsProvider = mGuests.front();
+        CollectorGuestList::iterator it;
+        /* Assume that nobody can provide VMM stats */
+        mVMMStatsProvider = NULL;
+
+        for (it = mGuests.begin(); it != mGuests.end(); it++)
+        {
+            /* Skip unregistered as they are about to be destroyed */
+            if ((*it)->isUnregistered())
+                continue;
+
+            if ((*it)->isEnabled())
+            {
+                /* Found the guest already collecting stats, elect it */
+                mVMMStatsProvider = *it;
+                break;
+            }
+            else if (!mVMMStatsProvider)
+            {
+                /* If nobody collects stats, take the first registered */
+                mVMMStatsProvider = *it;
+            }
         }
     }
@@ -286 +290 @@
 }
 
+void CollectorGuestManager::destroyUnregistered()
+{
+    CollectorGuestList::iterator it;
+
+    for (it = mGuests.begin(); it != mGuests.end();)
+        if ((*it)->isUnregistered())
+        {
+            delete *it;
+            it = mGuests.erase(it);
+            LogAleksey(("{%p} " LOG_FN_FMT ": Number of guests after erasing unregistered is %d\n",
+                        this, __PRETTY_FUNCTION__, mGuests.size()));
+        }
+        else
+            ++it;
+}
 
 #endif /* !VBOX_COLLECTOR_TEST_CASE */

trunk/src/VBox/Main/src-server/PerformanceImpl.cpp

@@ -16 +16 @@
  * VirtualBox OSE distribution. VirtualBox OSE is distributed in the
  * hope that it will be useful, but WITHOUT ANY WARRANTY of any kind.
+ */
+
+/*
+ * Rules of engagement:
+ * 1) All performance objects must be destroyed by PerformanceCollector only!
+ * 2) All public methods of PerformanceCollector must be protected with
+ *    read or write lock.
+ * 3) samplerCallback only uses the write lock during the third phase
+ *    which pulls data into SubMetric objects. This is where object destruction
+ *    and all list modifications are done. The pre-collection phases are
+ *    run without any locks which is only possible because:
+ * 4) Public methods of PerformanceCollector as well as pre-collection methods
+      cannot modify lists or destroy objects, and:
+ * 5) Pre-collection methods cannot modify metric data.
  */
 
@@ -200 +214 @@
     mMagic = 0;
 
+    /* Destroy unregistered metrics */
+    BaseMetricList::iterator it;
+    for (it = m.baseMetrics.begin(); it != m.baseMetrics.end();)
+        if ((*it)->isUnregistered())
+        {
+            delete *it;
+            it = m.baseMetrics.erase(it);
+        }
+        else
+            ++it;
+    Assert(m.baseMetrics.size() == 0);
+    /*
+     * Now when we have destroyed all base metrics that could
+     * try to pull data from unregistered CollectorGuest objects
+     * it is safe to destroy them as well.
+     */
+    m.gm->destroyUnregistered();
+
     /* Destroy resource usage sampler */
     int vrc = RTTimerLRDestroy (m.sampler);
@@ -534 +566 @@
 
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-    LogAleksey(("{%p} " LOG_FN_FMT ": before remove_if: m.baseMetrics.size()=%d\n", this, __PRETTY_FUNCTION__, m.baseMetrics.size()));
+    int n = 0;
     BaseMetricList::iterator it;
-    for (it = m.baseMetrics.begin(); it != m.baseMetrics.end();)
+    for (it = m.baseMetrics.begin(); it != m.baseMetrics.end(); ++it)
         if ((*it)->associatedWith(aObject))
         {
-            delete *it;
-            it = m.baseMetrics.erase(it);
+            (*it)->unregister();
+            ++n;
         }
-        else
-            ++it;
-    LogAleksey(("{%p} " LOG_FN_FMT ": after remove_if: m.baseMetrics.size()=%d\n", this, __PRETTY_FUNCTION__, m.baseMetrics.size()));
+    LogAleksey(("{%p} " LOG_FN_FMT ": obj=%p, marked %d metrics\n",
+                this, __PRETTY_FUNCTION__, (void *)aObject, n));
     //LogFlowThisFuncLeave();
 }
@@ -640 +671 @@
 {
     Log4(("{%p} " LOG_FN_FMT ": ENTER\n", this, __PRETTY_FUNCTION__));
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+    /* No locking until stage 3!*/
 
     pm::CollectorHints hints;
@@ -665 +696 @@
     m.gm->preCollect(hints, iTick);
 
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+    /*
+     * Before we can collect data we need to go through both lists
+     * again to see if any base metrics are marked as unregistered.
+     * Those should be destroyed now.
+     */
+    LogAleksey(("{%p} " LOG_FN_FMT ": before remove_if: toBeCollected.size()=%d\n", this, __PRETTY_FUNCTION__, toBeCollected.size()));
+    toBeCollected.remove_if(std::mem_fun(&pm::BaseMetric::isUnregistered));
+    LogAleksey(("{%p} " LOG_FN_FMT ": after remove_if: toBeCollected.size()=%d\n", this, __PRETTY_FUNCTION__, toBeCollected.size()));
+    LogAleksey(("{%p} " LOG_FN_FMT ": before remove_if: m.baseMetrics.size()=%d\n", this, __PRETTY_FUNCTION__, m.baseMetrics.size()));
+    for (it = m.baseMetrics.begin(); it != m.baseMetrics.end();)
+        if ((*it)->isUnregistered())
+        {
+            delete *it;
+            it = m.baseMetrics.erase(it);
+        }
+        else
+            ++it;
+    LogAleksey(("{%p} " LOG_FN_FMT ": after remove_if: m.baseMetrics.size()=%d\n", this, __PRETTY_FUNCTION__, m.baseMetrics.size()));
+    /*
+     * Now when we have destroyed all base metrics that could
+     * try to pull data from unregistered CollectorGuest objects
+     * it is safe to destroy them as well.
+     */
+    m.gm->destroyUnregistered();
+
     /* Finally, collect the data */
     std::for_each (toBeCollected.begin(), toBeCollected.end(),