Changeset 30847 in vbox

Timestamp:

Jul 14, 2010 3:40:49 PM (14 years ago)

Author:

vboxsync

Message:

Main/Machine+Performance+VirtualBox: move deleting the performance metrics registration of a VM out of SessionMachine::uninit, where it was a risk of hangs due to callbacks. Doing it a little later won't hurt anyone. Additionally also make sure the total VMM memory stats are zeroed when the last VM has disabled its collector.

Location:

trunk/src/VBox/Main

Files:

• MachineImpl.cpp (modified) (1 diff)
• Performance.cpp (modified) (3 diffs)
• VirtualBoxImpl.cpp (modified) (1 diff)
• include/MachineImpl.h (modified) (2 diffs)
• include/Performance.h (modified) (2 diffs)
• include/VirtualBoxImpl.h (modified) (1 diff)

trunk/src/VBox/Main/MachineImpl.cpp

@@ -9597 +9597 @@
     AutoMultiWriteLock3 multilock(mParent, mParent->host(), this COMMA_LOCKVAL_SRC_POS);
 
-#ifdef VBOX_WITH_RESOURCE_USAGE_API
-    unregisterMetrics(mParent->performanceCollector(), mPeer);
-#endif /* VBOX_WITH_RESOURCE_USAGE_API */
+    // 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);
 
     if (aReason == Uninit::Abnormal)

trunk/src/VBox/Main/Performance.cpp

@@ -120 +120 @@
 
 #ifndef VBOX_COLLECTOR_TEST_CASE
+
+uint32_t CollectorGuestHAL::cVMsEnabled = 0;
+
+CollectorGuestHAL::CollectorGuestHAL(Machine *machine, CollectorHAL *hostHAL)
+    : CollectorHAL(), cEnabled(0), mMachine(machine), mConsole(NULL),
+      mGuest(NULL), mLastTick(0), mHostHAL(hostHAL), mCpuUser(0),
+      mCpuKernel(0), mCpuIdle(0), mMemTotal(0), mMemFree(0),
+      mMemBalloon(0), mMemShared(0), mMemCache(0), mPageTotal(0)
+{
+    Assert(mMachine);
+    /* cannot use ComObjPtr<Machine> in Performance.h, do it manually */
+    mMachine->AddRef();
+}
+
 CollectorGuestHAL::~CollectorGuestHAL()
 {
+    /* cannot use ComObjPtr<Machine> in Performance.h, do it manually */
+    mMachine->Release();
     Assert(!cEnabled);
 }
@@ -127 +143 @@
 int CollectorGuestHAL::enable()
 {
+    /* Must make sure that the machine object does not get uninitialized
+     * in the middle of enabling this collector. Causes timing-related
+     * behavior otherwise, which we don't want. In particular the
+     * GetRemoteConsole call below can hang if the VM didn't completely
+     * terminate (the VM processes stop processing events shortly before
+     * closing the session). This avoids the hang. */
+    AutoCaller autoCaller(mMachine);
+    if (FAILED(autoCaller.rc())) return autoCaller.rc();
+
     HRESULT ret = S_OK;
 
     if (ASMAtomicIncU32(&cEnabled) == 1)
     {
+        ASMAtomicIncU32(&cVMsEnabled);
         ComPtr<IInternalSessionControl> directControl;
 
@@ -153 +179 @@
     if (ASMAtomicDecU32(&cEnabled) == 0)
     {
+        if (ASMAtomicDecU32(&cVMsEnabled) == 0)
+        {
+            if (mHostHAL)
+                mHostHAL->setMemHypervisorStats(0 /* ulMemAllocTotal */, 0 /* ulMemFreeTotal */, 0 /* ulMemBalloonTotal */, 0 /* ulMemSharedTotal */);
+        }
         Assert(mGuest && mConsole);
         mGuest->COMSETTER(StatisticsUpdateInterval)(0 /* off */);

trunk/src/VBox/Main/VirtualBoxImpl.cpp

@@ -2701 +2701 @@
 {
     postEvent(new GuestPropertyEvent(this, aMachineId, aName, aValue, aFlags));
+}
+
+/** Event for onMachineUninit(), this is not a CallbackEvent */
+class MachineUninitEvent : public Event
+{
+public:
+
+    MachineUninitEvent(VirtualBox *aVirtualBox, Machine *aMachine)
+        : mVirtualBox(aVirtualBox), mMachine(aMachine)
+    {
+        Assert(aVirtualBox);
+        Assert(aMachine);
+    }
+
+    void *handler()
+    {
+#ifdef VBOX_WITH_RESOURCE_USAGE_API
+        /* Handle unregistering metrics here, as it is not vital to get
+         * it done immediately. It reduces the number of locks needed and
+         * the lock contention in SessionMachine::uninit. */
+        mMachine->unregisterMetrics(mVirtualBox->performanceCollector(), mMachine);
+#endif /* VBOX_WITH_RESOURCE_USAGE_API */
+
+        return NULL;
+    }
+
+private:
+
+    /**
+     *  Note that this is a weak ref -- the CallbackEvent handler thread
+     *  is bound to the lifetime of the VirtualBox instance, so it's safe.
+     */
+    VirtualBox        *mVirtualBox;
+
+    /** Reference to the machine object. */
+    ComObjPtr<Machine> mMachine;
+};
+
+/**
+ *  Trigger internal event. This isn't meant to be signalled to clients.
+ *  @note Doesn't lock any object.
+ */
+void VirtualBox::onMachineUninit(Machine *aMachine)
+{
+    postEvent(new MachineUninitEvent(this, aMachine));
 }
 

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

@@ -356 +356 @@
 
     void uninit();
+
+#ifdef VBOX_WITH_RESOURCE_USAGE_API
+    // Needed from VirtualBox, for the delayed metrics cleanup.
+    void unregisterMetrics(PerformanceCollector *aCollector, Machine *aMachine);
+#endif /* VBOX_WITH_RESOURCE_USAGE_API */
 
 protected:
@@ -801 +806 @@
 #ifdef VBOX_WITH_RESOURCE_USAGE_API
     void registerMetrics(PerformanceCollector *aCollector, Machine *aMachine, RTPROCESS pid);
-    void unregisterMetrics(PerformanceCollector *aCollector, Machine *aMachine);
 
     pm::CollectorGuestHAL  *mGuestHAL;

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

@@ -186 +186 @@
     {
     public:
-        CollectorGuestHAL(Machine *machine, CollectorHAL *hostHAL) : CollectorHAL(), cEnabled(0), mMachine(machine), mConsole(NULL), mGuest(NULL),
-                                              mLastTick(0), mHostHAL(hostHAL), mCpuUser(0), mCpuKernel(0), mCpuIdle(0), mMemTotal(0), mMemFree(0),
-                                              mMemBalloon(0), mMemShared(0), mMemCache(0), mPageTotal(0) {};
+        CollectorGuestHAL(Machine *machine, CollectorHAL *hostHAL);
         ~CollectorGuestHAL();
 
@@ -236 +234 @@
         ULONG                mMemCache;
         ULONG                mPageTotal;
+
+    private:
+        static uint32_t      cVMsEnabled;
     };
 

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

@@ -215 +215 @@
     void onGuestPropertyChange(const Guid &aMachineId, IN_BSTR aName, IN_BSTR aValue,
                                IN_BSTR aFlags);
+    void onMachineUninit(Machine *aMachine);
 
     ComObjPtr<GuestOSType> getUnknownOSType();