Changeset 13729 in vbox

Timestamp:

Nov 1, 2008 1:43:27 PM (16 years ago)

Author:

vboxsync

Message:

Main: VirtualBoxBaseWithChildrenNEXT: Don't use a separate lock for a map since in the new locking scheme it is safe to use the general object lock. Add a possiblity to override the lock used to serialize access to the map (list for VirtualBoxBaseWithTypedChildrenNEXT) of children by reimplementing childrenLock().

Location:

trunk/src/VBox/Main

Files:

• VirtualBoxBase.cpp (modified) (2 diffs)
• include/VirtualBoxBase.h (modified) (21 diffs)

trunk/src/VBox/Main/VirtualBoxBase.cpp

@@ -1068 +1068 @@
 
 /**
- * Uninitializes all dependent children registered with #addDependentChild().
- *
- * Typically called from the uninit() method. Note that this method will call
- * uninit() methods of child objects. If these methods need to call the parent
- * object during initialization, uninitDependentChildren() must be called before
- * the relevant part of the parent is uninitialized, usually at the begnning of
- * the parent uninitialization sequence.
+ * Uninitializes all dependent children registered on this object with
+ * #addDependentChild().
+ *
+ * Must be called from within the VirtualBoxBaseProto::AutoUninitSpan (i.e.
+ * typically from this object's uninit() method) to uninitialize children
+ * before this object goes out of service and becomes unusable.
+ *
+ * Note that this method will call uninit() methods of child objects. If
+ * these methods need to call the parent object during uninitialization,
+ * #uninitDependentChildren() must be called before the relevant part of the
+ * parent is uninitialized: usually at the begnning of the parent
+ * uninitialization sequence.
+ *
+ * @note May lock something through the called children.
  */
 void VirtualBoxBaseWithChildrenNEXT::uninitDependentChildren()
 {
-    LogFlowThisFuncEnter();
-
-    AutoWriteLock mapLock (mMapLock);
-
-    LogFlowThisFunc (("count=%u...\n", mDependentChildren.size()));
+    AutoCaller autoCaller (this);
+
+    /* We don't want to hold the childrenLock() write lock here (necessary
+     * to protect mDependentChildren) when uninitializing children because
+     * we want to avoid a possible deadlock where we could get stuck in
+     * child->uninit() blocked by AutoUninitSpan waiting for the number of
+     * child's callers to drop to zero, while some caller is stuck in our
+     * removeDependentChild() method waiting for the write lock.
+     *
+     * The only safe place to not lock and keep accessing our data members
+     * is the InUninit state (no active call to our object may exist on
+     * another thread when we are in InUinint, provided that all such calls
+     * use the AutoCaller class of course). InUinint is also used as a flag
+     * by removeDependentChild() that prevents touching mDependentChildren
+     * from outside. Therefore, we assert.
+     */
+    AssertReturnVoid (autoCaller.state() == InUninit);
 
     if (mDependentChildren.size())
     {
-        /* We keep the lock until we have enumerated all children.
-         * Those ones that will try to call removeDependentChild() from a
-         * different thread will have to wait */
-
-        Assert (mUninitDoneSem == NIL_RTSEMEVENT);
-        int vrc = RTSemEventCreate (&mUninitDoneSem);
-        AssertRC (vrc);
-
-        Assert (mChildrenLeft == 0);
-        mChildrenLeft = mDependentChildren.size();
-
         for (DependentChildren::iterator it = mDependentChildren.begin();
-            it != mDependentChildren.end(); ++ it)
+             it != mDependentChildren.end(); ++ it)
         {
             VirtualBoxBase *child = (*it).second;
             Assert (child);
+
+            /* Note that if child->uninit() happens to be called on another
+             * thread right before us and is not yet finished, the second
+             * uninit() call will wait until the first one has done so
+             * (thanks to AutoUninitSpan). */
             if (child)
                 child->uninit();
         }
 
+        /* release all weak references we hold */
         mDependentChildren.clear();
     }
-
-    /* Wait until all children that called uninit() on their own on other
-     * threads but stuck waiting for the map lock in removeDependentChild() have
-     * finished uninitialization. */
-
-    if (mUninitDoneSem != NIL_RTSEMEVENT)
-    {
-        /* let stuck children run */
-        mapLock.leave();
-
-        LogFlowThisFunc (("Waiting for uninitialization of all children...\n"));
-
-        RTSemEventWait (mUninitDoneSem, RT_INDEFINITE_WAIT);
-
-        mapLock.enter();
-
-        RTSemEventDestroy (mUninitDoneSem);
-        mUninitDoneSem = NIL_RTSEMEVENT;
-        Assert (mChildrenLeft == 0);
-    }
-
-    LogFlowThisFuncLeave();
-}
-
-/**
- * Returns a pointer to the dependent child corresponding to the given
- * interface pointer (used as a key in the map of dependent children) or NULL
- * if the interface pointer doesn't correspond to any child registered using
- * #addDependentChild().
+}
+
+/**
+ * Returns a pointer to the dependent child (registered using
+ * #addDependentChild()) corresponding to the given interface pointer or NULL if
+ * the given pointer is unrelated.
+ *
+ * The relation is checked by using the given interface pointer as a key in the
+ * map of dependent children.
  *
  * Note that ComPtr <IUnknown> is used as an argument instead of IUnknown * in
@@ -1143 +1137 @@
  *
  * @param aUnk  Pointer to map to the dependent child object.
- * @return      Pointer to the dependent child object.
+ * @return      Pointer to the dependent VirtualBoxBase child object.
+ *
+ * @note Locks #childrenLock() for reading.
  */
 VirtualBoxBaseNEXT *
 VirtualBoxBaseWithChildrenNEXT::getDependentChild (const ComPtr <IUnknown> &aUnk)
 {
-    AssertReturn (!!aUnk, NULL);
-
-    AutoWriteLock alock (mMapLock);
+    AssertReturn (!aUnk.isNull(), NULL);
+
+    AutoCaller autoCaller (this);
 
     /* return NULL if uninitDependentChildren() is in action */
-    if (mUninitDoneSem != NIL_RTSEMEVENT)
+    if (autoCaller.state() == InUninit)
         return NULL;
+
+    AutoReadLock alock (childrenLock());
 
     DependentChildren::const_iterator it = mDependentChildren.find (aUnk);
     if (it == mDependentChildren.end())
         return NULL;
+
     return (*it).second;
 }
 
+/** Helper for addDependentChild(). */
 void VirtualBoxBaseWithChildrenNEXT::doAddDependentChild (
     IUnknown *aUnk, VirtualBoxBaseNEXT *aChild)
 {
-    AssertReturnVoid (aUnk && aChild);
-
-    AutoWriteLock alock (mMapLock);
-
-    if (mUninitDoneSem != NIL_RTSEMEVENT)
-    {
-        /* uninitDependentChildren() is being run. For this very unlikely case,
-         * we have to increase the number of children left, for symmetry with
-         * a later #removeDependentChild() call. */
-        ++ mChildrenLeft;
-        return;
-    }
+    AssertReturnVoid (aUnk != NULL);
+    AssertReturnVoid (aChild != NULL);
+
+    AutoCaller autoCaller (this);
+
+    /* sanity */
+    AssertReturnVoid (autoCaller.state() == InInit ||
+                      autoCaller.state() == Ready ||
+                      autoCaller.state() == Limited);
+
+    AutoWriteLock alock (childrenLock());
 
     std::pair <DependentChildren::iterator, bool> result =
         mDependentChildren.insert (DependentChildren::value_type (aUnk, aChild));
-    AssertMsg (result.second, ("Failed to insert a child to the map\n"));
-}
-
+    AssertMsg (result.second, ("Failed to insert child %p to the map\n", aUnk));
+}
+
+/** Helper for removeDependentChild(). */
 void VirtualBoxBaseWithChildrenNEXT::doRemoveDependentChild (IUnknown *aUnk)
 {
     AssertReturnVoid (aUnk);
 
-    AutoWriteLock alock (mMapLock);
-
-    if (mUninitDoneSem != NIL_RTSEMEVENT)
-    {
-        /* uninitDependentChildren() is being run. Just decrease the number of
-         * children left and signal a semaphore if it reaches zero. */
-        Assert (mChildrenLeft != 0);
-        -- mChildrenLeft;
-        if (mChildrenLeft == 0)
-        {
-            int vrc = RTSemEventSignal (mUninitDoneSem);
-            AssertRC (vrc);
-        }
+    AutoCaller autoCaller (this);
+
+    /* return shortly; uninitDependentChildren() will do the job */
+    if (autoCaller.state() == InUninit)
         return;
-    }
+
+    AutoWriteLock alock (childrenLock());
 
     DependentChildren::size_type result = mDependentChildren.erase (aUnk);
-    AssertMsg (result == 1, ("Failed to remove the child %p from the map\n",
-                             aUnk));
+    AssertMsg (result == 1, ("Failed to remove child %p from the map\n", aUnk));
     NOREF (result);
 }

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

@@ -1927 +1927 @@
  * <ol><li>
  *      Given an IUnknown instance, it's possible to quickly determine
- *      whether this instance represents a child object created by the given
- *      component, and if so, get a valid VirtualBoxBase pointer to the child
- *      object. The returned pointer can be then safely casted to the
+ *      whether this instance represents a child object that belongs to the
+ *      given component, and if so, get a valid VirtualBoxBase pointer to the
+ *      child object. The returned pointer can be then safely casted to the
  *      actual class of the child object (to get access to its "internal"
  *      non-interface methods) provided that no other child components implement
- *      the same initial interface IUnknown is queried from.
+ *      the same orignial COM interface IUnknown is queried from.
  * </li><li>
  *      When the parent object uninitializes itself, it can easily unintialize
@@ -1945 +1945 @@
  *      its parent to register itself within the list of dependent children.
  * </li><li>
- *      When a child object it is uninitialized, it calls
- *      #removeDependentChild() to unregister itself. Since the child's
- *      uninitialization may originate both from this method and from the child
- *      itself calling its uninit() on another thread at the same time, please
- *      make sure that #removeDependentChild() is called:
- *      <ul><li>
- *          after the child has successfully entered AutoUninitSpan -- to make
- *          sure this method is called only once for the given child object
- *          transitioning from Ready to NotReady. A failure to do so will at
- *          least likely cause an assertion ("Failed to remove the child from
- *          the map").
- *      </li><li>
- *          outside the child object's lock -- to avoid guaranteed deadlocks
- *          caused by different lock order: (child_lock, map_lock) in uninit()
- *          and (map_lock, child_lock) in this method.
- *      </li></ul>
+ *      When the child object it is uninitialized, it calls
+ *      #removeDependentChild() to unregister itself.
  * </li></ol>
+ *
+ * Note that if the parent object does not call #uninitDependentChildren() when
+ * it gets uninitialized, it must call uninit() methods of individual children
+ * manually to disconnect them; a failure to do so will cause crashes in these
+ * methods when chidren get destroyed.
  *
  * Note that children added by #addDependentChild() are <b>weakly</b> referenced
@@ -1969 +1960 @@
  * described here.
  *
- * @note Once again: because of weak referencing, deadlocks and assertions are
- *       very likely if #addDependentChild() or #removeDependentChild() are used
- *       incorrectly (called at inappropriate times). Check the above rules once
- *       more.
+ * Access to the child list is serialized using the #childrenLock() lock handle
+ * (which defaults to the general object lock handle (see
+ * VirtualBoxBase::lockHandle()). This lock is used by all add/remove methods of
+ * this class so be aware of the need to preserve the {parent, child} lock order
+ * when calling these methods.
  *
  * @todo This is a VirtualBoxBaseWithChildren equivalent that uses the
@@ -1984 +1976 @@
 
     VirtualBoxBaseWithChildrenNEXT()
-        : mUninitDoneSem (NIL_RTSEMEVENT), mChildrenLeft (0)
     {}
 
@@ -1991 +1982 @@
 
     /**
-     * Adds the given child to the map of dependent children.
-     *
-     * Typically called from the child's init() method, from within the
-     * AutoInitSpan scope.  Otherwise, VirtualBoxBase::AutoCaller must be
-     * used on @a aChild to make sure it is not uninitialized during this
-     * method's call.
+     * Lock handle to use when adding/removing child objects from the list of
+     * children. It is guaranteed that no any other lock is requested in methods
+     * of this class while holding this lock.
+     *
+     * @warning By default, this simply returns the general object's lock handle
+     *          (see VirtualBoxBase::lockHandle()) which is sufficient for most
+     *          cases.
+     */
+    virtual RWLockHandle *childrenLock() { return lockHandle(); }
+
+    /**
+     * Adds the given child to the list of dependent children.
+     *
+     * Usually gets called from the child's init() method.
+     *
+     * @note @a aChild (unless it is in InInit state) must be protected by
+     *       VirtualBoxBase::AutoCaller to make sure it is not uninitialized on
+     *       another thread during this method's call.
+     *
+     * @note When #childrenLock() is not overloaded (returns the general object
+     *       lock) and this method is called from under the child's read or
+     *       write lock, make sure the {parent, child} locking order is
+     *       preserved by locking the callee (this object) for writing before
+     *       the child's lock.
      *
      * @param aChild    Child object to add (must inherit VirtualBoxBase AND
      *                  implement some interface).
+     *
+     * @note Locks #childrenLock() for writing.
      */
     template <class C>
     void addDependentChild (C *aChild)
     {
-        AssertReturnVoid (aChild);
+        AssertReturnVoid (aChild != NULL);
         doAddDependentChild (ComPtr <IUnknown> (aChild), aChild);
     }
@@ -2020 +2031 @@
 
     /**
-     * Removes the given child from the map of dependent children.
-     *
-     * Typically called from from the child's uninit() method, from within the
-     * AutoUninitSpan scope. Make sure te child is not locked for reading or
-     * writing in this case.
-     *
-     * If called not from within the AutoUninitSpan scope,
-     * VirtualBoxBase::AutoCaller must be used on @a aChild to make sure it is
-     * not uninitialized during this method's call.
-     *
-     * @param aChild    Child object to remove (must inherit VirtualBoxBase AND
-     *                  implement some interface).
+     * Removes the given child from the list of dependent children.
+     *
+     * Usually gets called from the child's uninit() method.
+     *
+     * @note Locks #childrenLock() for writing.
+     *
+     * @note @a aChild (unless it is in InUninit state) must be protected by
+     *       VirtualBoxBase::AutoCaller to make sure it is not uninitialized on
+     *       another thread during this method's call.
+     *
+     * @note When #childrenLock() is not overloaded (returns the general object
+     *       lock) and this method is called from under the child's read or
+     *       write lock, make sure the {parent, child} locking order is
+     *       preserved by locking the callee (this object) for writing before
+     *       the child's lock. This is irrelevant when the method is called from
+     *       under this object's VirtualBoxBaseProto::AutoUninitSpan (i.e. in
+     *       InUninit state) since in this case no locking is done.
+     *
+     * @param aChild    Child object to remove.
+     *
+     * @note Locks #childrenLock() for writing.
      */
     template <class C>
     void removeDependentChild (C *aChild)
     {
-        AssertReturnVoid (aChild);
+        AssertReturnVoid (aChild != NULL);
         doRemoveDependentChild (ComPtr <IUnknown> (aChild));
     }
@@ -2072 +2092 @@
     typedef std::map <IUnknown *, VirtualBoxBaseNEXT *> DependentChildren;
     DependentChildren mDependentChildren;
-
-    RTSEMEVENT mUninitDoneSem;
-    size_t mChildrenLeft;
-
-    /* Protects all the fields above */
-    RWLockHandle mMapLock;
 };
 
@@ -2092 +2106 @@
  *  #addDependentChild() are <b>strongly</b> referenced, so that they cannot
  *  be externally destructed until #removeDependentChild() is called.
- *  For this reason, strict rules of calling #removeDependentChild() don't
- *  apply to instances of this class -- it can be called anywhere in the
- *  child's uninit() implementation.
+ *
+ *  Also, this class doesn't have the
+ *  VirtualBoxBaseWithChildrenNEXT::getDependentChild() method because it would
+ *  be not fast for long lists.
  *
  *  @param C    type of child objects (must inherit VirtualBoxBase AND
@@ -2238 +2253 @@
  * Base class to track component's chlidren of the particular type.
  *
- * This class is similar to VirtualBoxBaseWithChildren, with the exception that
- * all children must be of the same type. For this reason, it's not necessary to
- * use a map to store children -- a list is used instead.
+ * This class is similar to VirtualBoxBaseWithChildrenNEXT with the exception
+ * that all children must be of the same type. For this reason, it's not
+ * necessary to use a map to store children -- a list is used instead.
  *
  * Also, as opposed to VirtualBoxBaseWithChildren, children added by
@@ -2248 +2263 @@
  * See individual method descriptions for more information.
  *
- * @param C Type of child objects (must inherit VirtualBoxBase AND implementsome
- *          interface).
+ * @param C Type of child objects (must inherit VirtualBoxBase AND implement
+ *          some interface).
  *
  * @todo This is a VirtualBoxBaseWithChildren equivalent that uses the
@@ -2268 +2283 @@
 
     /**
+     * Lock handle to use when adding/removing child objects from the list of
+     * children. It is guaranteed that no any other lock is requested in methods
+     * of this class while holding this lock.
+     *
+     * @warning By default, this simply returns the general object's lock handle
+     *          (see VirtualBoxBase::lockHandle()) which is sufficient for most
+     *          cases.
+     */
+    virtual RWLockHandle *childrenLock() { return lockHandle(); }
+
+    /**
      * Adds the given child to the list of dependent children.
      *
      * Usually gets called from the child's init() method.
-     *
-     * @note Locks this object for writing.
      *
      * @note @a aChild (unless it is in InInit state) must be protected by
@@ -2278 +2302 @@
      *       another thread during this method's call.
      *
-     * @note If this method is called from under the child's read or write lock,
-     *       make sure the {parent, child} locking order is preserved by locking
-     *       the callee (this object) for writing before the child's lock.
+     * @note When #childrenLock() is not overloaded (returns the general object
+     *       lock) and this method is called from under the child's read or
+     *       write lock, make sure the {parent, child} locking order is
+     *       preserved by locking the callee (this object) for writing before
+     *       the child's lock.
      *
      * @param aChild    Child object to add.
+     *
+     * @note Locks #childrenLock() for writing.
      */
     void addDependentChild (C *aChild)
     {
-        AssertReturnVoid (aChild);
+        AssertReturnVoid (aChild != NULL);
 
         AutoCaller autoCaller (this);
@@ -2295 +2323 @@
                           autoCaller.state() == Limited);
 
-        AutoWriteLock alock (this);
+        AutoWriteLock alock (childrenLock());
         mDependentChildren.push_back (aChild);
     }
@@ -2304 +2332 @@
      * Usually gets called from the child's uninit() method.
      *
-     * Note that once this method returns, the callee (this object) is not
-     * guaranteed to be valid any more, so the caller must not call its
-     * other methods.
-     *
-     * @note Locks this object for writing.
+     * Note that once this method returns, the argument (@a aChild) is not
+     * guaranteed to be valid any more, so the caller of this method must not
+     * call its other methods.
      *
      * @note @a aChild (unless it is in InUninit state) must be protected by
@@ -2314 +2340 @@
      *       another thread during this method's call.
      *
-     * @note If this method is called from under the child's read or write lock,
-     *       make sure the {parent, child} locking order is preserved by locking
-     *       the callee (this object) for writing before the child's lock.
+     * @note When #childrenLock() is not overloaded (returns the general object
+     *       lock) and this method is called from under the child's read or
+     *       write lock, make sure the {parent, child} locking order is
+     *       preserved by locking the callee (this object) for writing before
+     *       the child's lock. This is irrelevant when the method is called from
+     *       under this object's AutoUninitSpan (i.e. in InUninit state) since
+     *       in this case no locking is done.
      *
      * @param aChild    Child object to remove.
+     *
+     * @note Locks #childrenLock() for writing.
      */
     void removeDependentChild (C *aChild)
@@ -2330 +2362 @@
             return;
 
-        AutoWriteLock alock (this);
+        AutoWriteLock alock (childrenLock());
         mDependentChildren.remove (aChild);
     }
@@ -2339 +2371 @@
      * Returns the read-only list of all dependent children.
      *
-     * @note Access the returned list (iterate, get size etc.) only after
-     *       locking this object for reading or for writing!
+     * @note Access the returned list (iterate, get size etc.) only after making
+     *       sure #childrenLock() is locked for reading or for writing!
      */
     const DependentChildren &dependentChildren() const { return mDependentChildren; }
@@ -2355 +2387 @@
      * these methods need to call the parent object during uninitialization,
      * #uninitDependentChildren() must be called before the relevant part of the
-     * parent is uninitialized, usually at the begnning of the parent
+     * parent is uninitialized: usually at the begnning of the parent
      * uninitialization sequence.
      *
-     * @note May lock this object through the called children.
+     * @note May lock something through the called children.
      */
     void uninitDependentChildren()
@@ -2364 +2396 @@
         AutoCaller autoCaller (this);
 
-        /* We cannot hold the write lock (necessary here to protect
-         * mDependentChildren) when uninitializing children because we want to
-         * avoid a possible deadlock where we could get stuck in child->uninit()
-         * blocked by AutoUninitSpan waiting for the number of child's callers
-         * to drop to zero, while some caller is stuck in our
+        /* We don't want to hold the childrenLock() write lock here (necessary
+         * to protect mDependentChildren) when uninitializing children because
+         * we want to avoid a possible deadlock where we could get stuck in
+         * child->uninit() blocked by AutoUninitSpan waiting for the number of
+         * child's callers to drop to zero, while some caller is stuck in our
          * removeDependentChild() method waiting for the write lock.
          *
@@ -2389 +2421 @@
 
                 /* Note that if child->uninit() happens to be called on another
-                 * thread right before us and is not yet finished; the second
+                 * thread right before us and is not yet finished, the second
                  * uninit() call will wait until the first one has done so
                  * (thanks to AutoUninitSpan). */
@@ -2409 +2441 @@
      *       another thread during this method's call.
      *
-     * @note Locks this object for writing.
+     * @note Locks #childrenLock() for writing.
      */
     void removeDependentChildren()
     {
-        AutoWriteLock alock (this);
+        AutoWriteLock alock (childrenLock());
         mDependentChildren.clear();
     }