Changeset 40257 in vbox for trunk/src/VBox/Main/src-client

Timestamp:

Feb 27, 2012 9:25:12 AM (13 years ago)

Author:

vboxsync

Message:

Main/Medium: rework locking scheme to solve lock order violations and long GUI start up time caused by too much locking
Main/all: Remove the enter and leave methods from write locks, they cause hard to find locking problems. Better solve them explicitly.

Location:

trunk/src/VBox/Main/src-client

Files:

• ConsoleImpl.cpp (modified) (51 diffs)
• ConsoleImplTeleporter.cpp (modified) (3 diffs)
• DisplayImpl.cpp (modified) (14 diffs)
• SessionImpl.cpp (modified) (3 diffs)

trunk/src/VBox/Main/src-client/ConsoleImpl.cpp

@@ -2077 +2077 @@
         return ptrVM.rc();
 
-    /* leave the lock before a VMR3* call (EMT will call us back)! */
-    alock.leave();
+    /* release the lock before a VMR3* call (EMT will call us back)! */
+    alock.release();
 
     int vrc = VMR3Reset(ptrVM);
@@ -2290 +2290 @@
                           this, pVM, aCpu);
 
-    /* leave the lock before a VMR3* call (EMT will call us back)! */
+    /* release the lock before a VMR3* call (EMT will call us back)! */
     alock.release();
 
@@ -2354 +2354 @@
     LogFlowThisFunc(("Sending PAUSE request...\n"));
 
-    /* leave the lock before a VMR3* call (EMT will call us back)! */
-    alock.leave();
+    /* release the lock before a VMR3* call (EMT will call us back)! */
+    alock.release();
 
     int vrc = VMR3Suspend(ptrVM);
@@ -2389 +2389 @@
     LogFlowThisFunc(("Sending RESUME request...\n"));
 
-    /* leave the lock before a VMR3* call (EMT will call us back)! */
-    alock.leave();
+    /* release the lock before a VMR3* call (EMT will call us back)! */
+    alock.release();
 
 #ifdef VBOX_WITH_EXTPACK
@@ -2434 +2434 @@
     if (!ptrVM.isOk())
         return ptrVM.rc();
-/** @todo leave the console lock? */
+
+/** @todo release the console lock? */
 
     /* get the acpi device interface and press the button. */
@@ -2481 +2482 @@
     if (!ptrVM.isOk())
         return ptrVM.rc();
-/** @todo leave the console lock? */
+
+/** @todo release the console lock? */
 
     /* get the acpi device interface and check if the button press was handled. */
@@ -2536 +2538 @@
         return ptrVM.rc();
 
-/** @todo leave the console lock? */
+/** @todo release the console lock? */
 
     /* get the acpi device interface and query the information. */
@@ -2577 +2579 @@
         return ptrVM.rc();
 
-/** @todo leave the console lock? */
+/** @todo release the console lock? */
 
     /* get the acpi device interface and press the sleep button. */
@@ -2679 +2681 @@
         /*
          * If we fail here it means a PowerDown() call happened on another
-         * thread while we were doing Pause() (which leaves the Console lock).
+         * thread while we were doing Pause() (which releases the Console lock).
          * We assign PowerDown() a higher precedence than SaveState(),
          * therefore just return the error to the caller.
@@ -2887 +2889 @@
             tr("The virtual machine does not have a USB controller"));
 
-    /* leave the lock because the USB Proxy service may call us back
+    /* release the lock because the USB Proxy service may call us back
      * (via onUSBDeviceAttach()) */
-    alock.leave();
+    alock.release();
 
     /* Request the device capture */
@@ -2932 +2934 @@
      * Inform the USB device and USB proxy about what's cooking.
      */
-    alock.leave();
+    alock.release();
     HRESULT rc2 = mControl->DetachUSBDevice(aId, false /* aDone */);
     if (FAILED(rc2))
         return rc2;
-    alock.enter();
+    alock.acquire();
 
     /* Request the PDM to detach the USB device. */
@@ -2943 +2945 @@
     if (SUCCEEDED(rc))
     {
-        /* leave the lock since we don't need it any more (note though that
+        /* release the lock since we don't need it any more (note though that
          * the USB Proxy service must not call us back here) */
-        alock.leave();
+        alock.release();
 
         /* Request the device release. Even if it fails, the device will
@@ -3269 +3271 @@
         /*
          * If we fail here it means a PowerDown() call happened on another
-         * thread while we were doing Pause() (which leaves the Console lock).
+         * thread while we were doing Pause() (which releases the Console lock).
          * We assign PowerDown() a higher precedence than TakeSnapshot(),
          * therefore just return the error to the caller.
@@ -3595 +3597 @@
                           fForce);
 
-    /* leave the lock before waiting for a result (EMT will call us back!) */
-    alock.leave();
+    /* release the lock before waiting for a result (EMT will call us back!) */
+    alock.release();
 
     if (vrc == VERR_TIMEOUT || RT_SUCCESS(vrc))
@@ -3848 +3850 @@
                           aMediumAttachment);
 
-    /* leave the lock before waiting for a result (EMT will call us back!) */
-    alock.leave();
+    /* release the lock before waiting for a result (EMT will call us back!) */
+    alock.release();
 
     if (vrc == VERR_TIMEOUT || RT_SUCCESS(vrc))
@@ -4089 +4091 @@
                           aMediumAttachment);
 
-    /* leave the lock before waiting for a result (EMT will call us back!) */
-    alock.leave();
+    /* release the lock before waiting for a result (EMT will call us back!) */
+    alock.release();
 
     if (vrc == VERR_TIMEOUT || RT_SUCCESS(vrc))
@@ -4498 +4500 @@
                           this, ptrVM.raw(), pszDevice, uInstance, uLun, aNetworkAdapter);
 
-    /* leave the lock before waiting for a result (EMT will call us back!) */
-    alock.leave();
+    /* release the lock before waiting for a result (EMT will call us back!) */
+    alock.release();
 
     if (vrc == VERR_TIMEOUT || RT_SUCCESS(vrc))
@@ -4833 +4835 @@
         {
             /* VRDP server may call this Console object back from other threads (VRDP INPUT or OUTPUT). */
-            alock.leave();
+            alock.release();
 
             if (vrdpEnabled)
@@ -4852 +4854 @@
             }
 
-            alock.enter();
+            alock.acquire();
         }
     }
@@ -6688 +6690 @@
     /* ----------------------------------------------------------------------
      * DONE with necessary state changes, perform the power down actions (it's
-     * safe to leave the object lock now if needed)
+     * safe to release the object lock now if needed)
      * ---------------------------------------------------------------------- */
 
@@ -6699 +6701 @@
         /* Leave the lock since EMT will call us back as addVMCaller()
          * in updateDisplayData(). */
-        alock.leave();
+        alock.release();
 
         mConsoleVRDPServer->Stop();
 
-        alock.enter();
+        alock.acquire();
     }
 
@@ -6729 +6731 @@
                           mVMCallers));
 
-        alock.leave();
+        alock.release();
 
         RTSemEventWait(mVMZeroCallersSem, RT_INDEFINITE_WAIT);
 
-        alock.enter();
+        alock.acquire();
     }
 
@@ -6753 +6755 @@
     {
         LogFlowThisFunc(("Powering off the VM...\n"));
-        alock.leave();
+        alock.release();
         vrc = VMR3PowerOff(VMR3GetVM(pUVM));
 #ifdef VBOX_WITH_EXTPACK
         mptrExtPackManager->callAllVmPowerOffHooks(this, VMR3GetVM(pUVM));
 #endif
-        alock.enter();
+        alock.acquire();
     }
 
@@ -6772 +6774 @@
 
         /* Leave the lock since EMT will call us back as addVMCaller() */
-        alock.leave();
+        alock.release();
 
         m_pVMMDev->hgcmShutdown();
 
-        alock.enter();
+        alock.acquire();
     }
 
@@ -6805 +6807 @@
 
         /* Now we've got to destroy the VM as well. (mpVM is not valid beyond
-         * this point). We leave the lock before calling VMR3Destroy() because
+         * this point). We release the lock before calling VMR3Destroy() because
          * it will result into calling destructors of drivers associated with
          * Console children which may in turn try to lock Console (e.g. by
@@ -6818 +6820 @@
         LogFlowThisFunc(("Destroying the VM...\n"));
 
-        alock.leave();
+        alock.release();
 
         vrc = VMR3Destroy(VMR3GetVM(pUVM));
 
         /* take the lock again */
-        alock.enter();
+        alock.acquire();
 
         /* advance percent count */
@@ -6930 +6932 @@
              * UpdateState we will require Machine and SessionMachine locks
              * (remember that here we're holding the Console lock here, and also
-             * all locks that have been entered by the thread before calling
+             * all locks that have been acquire by the thread before calling
              * this method).
              */
@@ -7648 +7650 @@
     AssertReturn(isWriteLockOnCurrentThread(), E_FAIL);
 
-    /* still want a lock object because we need to leave it */
+    /* still want a lock object because we need to release it */
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
@@ -7680 +7682 @@
                       Address.c_str(), uuid.raw()));
 
-    /* leave the lock before a VMR3* call (EMT will call us back)! */
-    alock.leave();
+    /* release the lock before a VMR3* call (EMT will call us back)! */
+    alock.release();
 
 /** @todo just do everything here and only wrap the PDMR3Usb call. That'll offload some notification stuff from the EMT thread. */
@@ -7689 +7691 @@
 
     /* restore the lock */
-    alock.enter();
+    alock.acquire();
 
     /* hrc is S_OK here */
@@ -7790 +7792 @@
     AssertReturn(isWriteLockOnCurrentThread(), E_FAIL);
 
-    /* still want a lock object because we need to leave it */
+    /* still want a lock object because we need to release it */
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
@@ -7804 +7806 @@
                      (*aIt)->id().raw()));
 
-    /* leave the lock before a VMR3* call (EMT will call us back)! */
-    alock.leave();
+    /* release the lock before a VMR3* call (EMT will call us back)! */
+    alock.release();
 
 /** @todo just do everything here and only wrap the PDMR3Usb call. That'll offload some notification stuff from the EMT thread. */
@@ -8266 +8268 @@
     if (RT_SUCCESS(vrc))
     {
-        /* leave the lock before calling Host in VBoxSVC since Host may call
+        /* release the lock before calling Host in VBoxSVC since Host may call
          * us back from under its lock (e.g. onUSBDeviceAttach()) which would
          * produce an inter-process dead-lock otherwise. */
         AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-        alock.leave();
+        alock.release();
 
         HRESULT hrc = mControl->AutoCaptureUSBDevices();
@@ -8300 +8302 @@
     mUSBDevices.clear();
 
-    /* leave the lock before calling Host in VBoxSVC since Host may call
+    /* release the lock before calling Host in VBoxSVC since Host may call
      * us back from under its lock (e.g. onUSBDeviceAttach()) which would
      * produce an inter-process dead-lock otherwise. */
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-    alock.leave();
+    alock.release();
 
     mControl->DetachAllUSBDevices(aDone);
@@ -8575 +8577 @@
 
         /* Does VRDP server call Console from the other thread?
-         * Not sure (and can change), so leave the lock just in case.
+         * Not sure (and can change), so release the lock just in case.
          */
-        alock.leave();
+        alock.release();
         vrc = server->Launch();
-        alock.enter();
+        alock.acquire();
 
         if (vrc == VERR_NET_ADDRESS_IN_USE)
@@ -8627 +8629 @@
         PVM pVM;
         /*
-         * leave the lock since EMT will call Console. It's safe because
+         * release the lock since EMT will call Console. It's safe because
          * mMachineState is either Starting or Restoring state here.
          */
-        alock.leave();
+        alock.release();
 
         vrc = VMR3Create(cCpus,
@@ -8640 +8642 @@
                          &pVM);
 
-        alock.enter();
+        alock.acquire();
 
         /* Enable client connections to the server. */
@@ -8677 +8679 @@
                 {
                     /* Does the code below call Console from the other thread?
-                     * Not sure, so leave the lock just in case. */
-                    alock.leave();
+                     * Not sure, so release the lock just in case. */
+                    alock.release();
 
                     for (SharedFolderDataMap::const_iterator it = task->mSharedFolders.begin();
@@ -8698 +8700 @@
                         rc = S_OK;          // do not fail with broken shared folders
 
-                    /* enter the lock again */
-                    alock.enter();
+                    /* acquire the lock again */
+                    alock.acquire();
                 }
 
@@ -8708 +8710 @@
                 if (FAILED(rc)) break;
 
-                /* leave the lock before a lengthy operation */
-                alock.leave();
+                /* release the lock before a lengthy operation */
+                alock.release();
 
                 /* Load saved state? */
@@ -8822 +8824 @@
                 }
 
-                /* enter the lock again */
-                alock.enter();
+                /* acquire the lock again */
+                alock.acquire();
             }
             while (0);
@@ -8835 +8837 @@
                 /* powerDown() will call VMR3Destroy() and do all necessary
                  * cleanup (VRDP, USB devices) */
+                alock.release();
                 HRESULT rc2 = pConsole->powerDown();
+                alock.acquire();
                 AssertComRC(rc2);
             }
@@ -8845 +8849 @@
                  * be sticky but our error callback isn't.
                  */
-                alock.leave();
+                alock.release();
                 VMR3AtErrorDeregister(pVM, Console::genericVMSetErrorCallback, &task->mErrorMsg);
                 /** @todo register another VMSetError callback? */
-                alock.enter();
+                alock.acquire();
             }
         }
@@ -8914 +8918 @@
      */
 
-    /* leave the lock, don't need it any more */
-    alock.leave();
+    /* release the lock, don't need it any more */
+    alock.release();
 
     if (SUCCEEDED(rc))
@@ -9114 +9118 @@
             pTask->mProgress->setCancelCallback(takesnapshotProgressCancelCallback, ptrVM.rawUVM());
 
-            alock.leave();
+            alock.release();
             LogFlowFunc(("VMR3Save...\n"));
             int vrc = VMR3Save(ptrVM,
@@ -9122 +9126 @@
                                static_cast<IProgress *>(pTask->mProgress),
                                &fSuspenededBySave);
-            alock.enter();
+            alock.acquire();
             if (RT_FAILURE(vrc))
                 throw setErrorStatic(E_FAIL,
@@ -9190 +9194 @@
 
                 /*
-                 * don't leave the lock since reconfigureMediumAttachment
+                 * don't release the lock since reconfigureMediumAttachment
                  * isn't going to need the Console lock.
                  */
@@ -9265 +9269 @@
                 LogFlowFunc(("VMR3Resume...\n"));
                 SafeVMPtr ptrVM(that);
-                alock.leave();
+                alock.release();
                 int vrc = VMR3Resume(ptrVM);
-                alock.enter();
+                alock.acquire();
                 if (RT_FAILURE(vrc))
                 {
@@ -9323 +9327 @@
                         LogFlowFunc(("VMR3Resume (on failure)...\n"));
                         SafeVMPtr ptrVM(that);
-                        alock.leave();
+                        alock.release();
                         int vrc = VMR3Resume(ptrVM); AssertLogRelRC(vrc);
-                        alock.enter();
+                        alock.acquire();
                         if (RT_FAILURE(vrc))
                             that->setMachineState(MachineState_Paused);
@@ -9411 +9415 @@
          */
         task->releaseVMCaller();
+        thatLock.release();
         rc = that->powerDown();
+        thatLock.acquire();
     }
 
@@ -9458 +9464 @@
     /* release VM caller to avoid the powerDown() deadlock */
     task->releaseVMCaller();
+
+    thatLock.release();
 
     that->powerDown(task->mServerProgress);

trunk/src/VBox/Main/src-client/ConsoleImplTeleporter.cpp

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2010 Oracle Corporation
+ * Copyright (C) 2010-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -886 +886 @@
                         if (pState->mfSuspendedByUs)
                         {
-                            autoLock.leave();
+                            autoLock.release();
                             int rc = VMR3Resume(VMR3GetVM(pState->mpUVM));
                             AssertLogRelMsgRC(rc, ("VMR3Resume -> %Rrc\n", rc));
-                            autoLock.enter();
+                            autoLock.acquire();
                         }
                     }
@@ -901 +901 @@
         }
     }
-    autoLock.leave();
+    autoLock.release();
 
     /*

trunk/src/VBox/Main/src-client/DisplayImpl.cpp

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2006-2010 Oracle Corporation
+ * Copyright (C) 2006-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -2038 +2038 @@
     if (pVM.isOk())
     {
-        /* Must leave the lock here because the changeFramebuffer will
+        /* Must release the lock here because the changeFramebuffer will
          * also obtain it. */
-        alock.leave ();
+        alock.release();
 
         /* send request to the EMT thread */
@@ -2046 +2046 @@
                                    (PFNRT) changeFramebuffer, 3, this, aFramebuffer, aScreenId);
 
-        alock.enter ();
+        alock.acquire();
 
         ComAssertRCRet (vrc, E_FAIL);
@@ -2064 +2064 @@
                 VMMDev *pVMMDev = mParent->getVMMDev();
 
-                alock.leave ();
+                alock.release();
 
                 if (pVMMDev)
@@ -2070 +2070 @@
                 /*ComAssertRCRet (vrc, E_FAIL);*/
 
-                alock.enter ();
+                alock.acquire();
             }
         }
@@ -2155 +2155 @@
 //        return setError(E_FAIL, tr("Not enough VRAM for the selected video mode"));
 
-    /* Have to leave the lock because the pfnRequestDisplayChange
+    /* Have to release the lock because the pfnRequestDisplayChange
      * will call EMT.  */
-    alock.leave ();
+    alock.release();
 
     VMMDev *pVMMDev = mParent->getVMMDev();
@@ -2176 +2176 @@
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
-    /* Have to leave the lock because the pfnRequestSeamlessChange will call EMT.  */
-    alock.leave ();
+    /* Have to release the lock because the pfnRequestSeamlessChange will call EMT.  */
+    alock.release();
 
     VMMDev *pVMMDev = mParent->getVMMDev();
@@ -2377 +2377 @@
     LogRelFlowFunc (("Sending SCREENSHOT request\n"));
 
-    /* Leave lock because other thread (EMT) is called and it may initiate a resize
+    /* Release lock because other thread (EMT) is called and it may initiate a resize
      * which also needs lock.
      *
      * This method does not need the lock anymore.
      */
-    alock.leave();
+    alock.release();
 
     int vrc = displayTakeScreenshot(pVM, this, mpDrv, aScreenId, address, width, height);
@@ -2430 +2430 @@
     LogRelFlowFunc (("Sending SCREENSHOT request\n"));
 
-    /* Leave lock because other thread (EMT) is called and it may initiate a resize
+    /* Release lock because other thread (EMT) is called and it may initiate a resize
      * which also needs lock.
      *
      * This method does not need the lock anymore.
      */
-    alock.leave();
+    alock.release();
 
     size_t cbData = width * 4 * height;
@@ -2507 +2507 @@
     LogRelFlowFunc (("Sending SCREENSHOT request\n"));
 
-    /* Leave lock because other thread (EMT) is called and it may initiate a resize
+    /* Release lock because other thread (EMT) is called and it may initiate a resize
      * which also needs lock.
      *
      * This method does not need the lock anymore.
      */
-    alock.leave();
+    alock.release();
 
     size_t cbData = width * 4 * height;
@@ -2676 +2676 @@
     if (FAILED(pVM.rc())) return pVM.rc();
 
-    /* Leave lock because the call scheduled on EMT may also try to take it. */
-    alock.leave();
+    /* Release lock because the call scheduled on EMT may also try to take it. */
+    alock.release();
 
     /*
@@ -2800 +2800 @@
     LogRelFlowFunc (("Sending DPYUPDATE request\n"));
 
-    /* Have to leave the lock when calling EMT.  */
-    alock.leave ();
+    /* Have to release the lock when calling EMT.  */
+    alock.release();
 
     /* pdm.h says that this has to be called from the EMT thread */
     int rcVBox = VMR3ReqCallVoidWait(pVM, VMCPUID_ANY, (PFNRT)Display::InvalidateAndUpdateEMT,
                                      1, this);
-    alock.enter ();
+    alock.acquire();
 
     if (RT_FAILURE(rcVBox))
@@ -3003 +3003 @@
 
 #if defined(VBOX_WITH_CROGL)
-        /* Leave the lock, because SHCRGL_HOST_FN_SCREEN_CHANGED will read current framebuffer */
+        /* Release the lock, because SHCRGL_HOST_FN_SCREEN_CHANGED will read current framebuffer */
         {
             BOOL is3denabled;
@@ -3010 +3010 @@
             if (is3denabled)
             {
-                alock.leave ();
+                alock.release();
             }
         }

trunk/src/VBox/Main/src-client/SessionImpl.cpp

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2006-2010 Oracle Corporation
+ * Copyright (C) 2006-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -936 +936 @@
          *  SessionState_Closing here, so it's safe.
          */
-        alock.leave();
+        alock.release();
 
         LogFlowThisFunc(("Calling mControl->OnSessionEnd()...\n"));
@@ -942 +942 @@
         LogFlowThisFunc(("mControl->OnSessionEnd()=%08X\n", rc));
 
-        alock.enter();
+        alock.acquire();
 
         /*