Changeset 27883 in vbox

Timestamp:

Mar 31, 2010 12:07:15 PM (15 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

59570

Message:

AsyncCompletion: Replace our own task cache with a RTMemCache. The old code had a race condition

Location:

trunk/src/VBox/VMM

Files:

• PDMAsyncCompletion.cpp (modified) (13 diffs)
• PDMAsyncCompletionInternal.h (modified) (3 diffs)

trunk/src/VBox/VMM/PDMAsyncCompletion.cpp

@@ -116 +116 @@
 } PDMASYNCCOMPLETIONTEMPLATE;
 
-static void pdmR3AsyncCompletionPutTask(PPDMASYNCCOMPLETIONENDPOINT pEndpoint, PPDMASYNCCOMPLETIONTASK pTask, bool fLocal);
+static void pdmR3AsyncCompletionPutTask(PPDMASYNCCOMPLETIONENDPOINT pEndpoint, PPDMASYNCCOMPLETIONTASK pTask);
 
 /**
@@ -580 +580 @@
     }
 
-    pdmR3AsyncCompletionPutTask(pTask->pEndpoint, pTask, true);
+    pdmR3AsyncCompletionPutTask(pTask->pEndpoint, pTask);
 }
 
@@ -620 +620 @@
             PCFGMNODE pCfgNodeClass = CFGMR3GetChild(pCfgHandle, pEpClassOps->pcszName);
 
-            /* Query the common CFGM options */
-            rc = CFGMR3QueryU32Def(pCfgNodeClass, "TaskCachePerEndpoint", &pEndpointClass->cEndpointCacheSize, 5);
-            AssertRCReturn(rc, rc);
-
-            rc = CFGMR3QueryU32Def(pCfgNodeClass, "TaskCachePerClass", &pEndpointClass->cEpClassCacheSize, 50);
-            AssertRCReturn(rc, rc);
-
-            /* Call the specific endpoint class initializer. */
-            rc = pEpClassOps->pfnInitialize(pEndpointClass, pCfgNodeClass);
+            /* Create task cache */
+            rc = RTMemCacheCreate(&pEndpointClass->hMemCacheTasks, pEpClassOps->cbTask,
+                                  0, UINT32_MAX, NULL, NULL, NULL, 0);
             if (RT_SUCCESS(rc))
             {
-                AssertMsg(!pVM->pdm.s.papAsyncCompletionEndpointClass[pEpClassOps->enmClassType],
-                              ("Endpoint class was already initialized\n"));
-
-                pVM->pdm.s.papAsyncCompletionEndpointClass[pEpClassOps->enmClassType] = pEndpointClass;
-                LogFlowFunc((": Initialized endpoint class \"%s\" rc=%Rrc\n", pEpClassOps->pcszName, rc));
-                return VINF_SUCCESS;
+                /* Call the specific endpoint class initializer. */
+                rc = pEpClassOps->pfnInitialize(pEndpointClass, pCfgNodeClass);
+                if (RT_SUCCESS(rc))
+                {
+                    AssertMsg(!pVM->pdm.s.papAsyncCompletionEndpointClass[pEpClassOps->enmClassType],
+                                  ("Endpoint class was already initialized\n"));
+
+                    pVM->pdm.s.papAsyncCompletionEndpointClass[pEpClassOps->enmClassType] = pEndpointClass;
+                    LogFlowFunc((": Initialized endpoint class \"%s\" rc=%Rrc\n", pEpClassOps->pcszName, rc));
+                    return VINF_SUCCESS;
+                }
+                RTMemCacheDestroy(pEndpointClass->hMemCacheTasks);
             }
             RTCritSectDelete(&pEndpointClass->CritSect);
@@ -665 +665 @@
         PDMR3AsyncCompletionEpClose(pEndpointClass->pEndpointsHead);
 
-    /* Destroy all cached tasks. */
-    for (unsigned i = 0; i < RT_ELEMENTS(pEndpointClass->apTaskCache); i++)
-    {
-        PPDMASYNCCOMPLETIONTASK pTask = pEndpointClass->apTaskCache[i];
-
-        while (pTask)
-        {
-            PPDMASYNCCOMPLETIONTASK pTaskFree = pTask;
-            pTask = pTask->pNext;
-            MMR3HeapFree(pTaskFree);
-        }
-    }
-
     /* Call the termination callback of the class. */
     pEndpointClass->pEndpointOps->pfnTerminate(pEndpointClass);
 
+    RTMemCacheDestroy(pEndpointClass->hMemCacheTasks);
     RTCritSectDelete(&pEndpointClass->CritSect);
 
@@ -754 +742 @@
 static PPDMASYNCCOMPLETIONTASK pdmR3AsyncCompletionGetTask(PPDMASYNCCOMPLETIONENDPOINT pEndpoint, void *pvUser)
 {
+    PPDMASYNCCOMPLETIONEPCLASS pEndpointClass = pEndpoint->pEpClass;
     PPDMASYNCCOMPLETIONTASK pTask = NULL;
 
-    /* Try the small per endpoint cache first. */
-    uint32_t cTasksCached = ASMAtomicReadU32(&pEndpoint->cTasksCached);
-    if (cTasksCached == 0)
-    {
-        /* Try the bigger per endpoint class cache. */
-        PPDMASYNCCOMPLETIONEPCLASS pEndpointClass = pEndpoint->pEpClass;
-
-        /* We start with the assigned slot id to distribute the load when allocating new tasks. */
-        unsigned iSlot = pEndpoint->iSlotStart;
-        do
-        {
-            pTask = (PPDMASYNCCOMPLETIONTASK)ASMAtomicXchgPtr((void * volatile *)&pEndpointClass->apTaskCache[iSlot], NULL);
-            if (pTask)
-                break;
-
-            iSlot = (iSlot + 1) % RT_ELEMENTS(pEndpointClass->apTaskCache);
-        } while (iSlot != pEndpoint->iSlotStart);
-
-        if (!pTask)
-        {
-            /*
-             * Allocate completely new.
-             * If this fails we return NULL.
-             */
-            int rc = MMR3HeapAllocZEx(pEndpointClass->pVM, MM_TAG_PDM_ASYNC_COMPLETION,
-                                      pEndpointClass->pEndpointOps->cbTask,
-                                      (void **)&pTask);
-            if (RT_FAILURE(rc))
-                pTask = NULL;
-        }
-        else
-        {
-            /* Remove the first element and put the rest into the slot again. */
-            PPDMASYNCCOMPLETIONTASK pTaskHeadNew = pTask->pNext;
-
-            /* Put back into the list adding any new tasks. */
-            while (true)
-            {
-                bool fChanged = ASMAtomicCmpXchgPtr((void * volatile *)&pEndpointClass->apTaskCache[iSlot], pTaskHeadNew, NULL);
-
-                if (fChanged)
-                    break;
-
-                PPDMASYNCCOMPLETIONTASK pTaskHead = (PPDMASYNCCOMPLETIONTASK)ASMAtomicXchgPtr((void * volatile *)&pEndpointClass->apTaskCache[iSlot], NULL);
-
-                /* The new task could be taken inbetween */
-                if (pTaskHead)
-                {
-                    /* Go to the end of the probably much shorter new list. */
-                    PPDMASYNCCOMPLETIONTASK pTaskTail = pTaskHead;
-                    while (pTaskTail->pNext)
-                        pTaskTail = pTaskTail->pNext;
-
-                    /* Concatenate */
-                    pTaskTail->pNext = pTaskHeadNew;
-
-                    pTaskHeadNew = pTaskHead;
-                }
-                /* Another round trying to change the list. */
-            }
-            /* We got a task from the global cache so decrement the counter */
-            ASMAtomicDecU32(&pEndpointClass->cTasksCached);
-        }
-    }
-    else
-    {
-        /* Grab a free task from the head. */
-        AssertMsg(pEndpoint->cTasksCached > 0, ("No tasks cached but list contain more than one element\n"));
-
-        pTask = pEndpoint->pTasksFreeHead;
-        pEndpoint->pTasksFreeHead = pTask->pNext;
-        ASMAtomicDecU32(&pEndpoint->cTasksCached);
-    }
+    pTask = (PPDMASYNCCOMPLETIONTASK)RTMemCacheAlloc(pEndpointClass->hMemCacheTasks);
 
     if (RT_LIKELY(pTask))
@@ -856 +773 @@
  * @param   pEndpoint    The endpoint the task belongs to.
  * @param   pTask        The task to cache.
- * @param   fLocal       Whether the per endpoint cache should be tried first.
- */
-static void pdmR3AsyncCompletionPutTask(PPDMASYNCCOMPLETIONENDPOINT pEndpoint, PPDMASYNCCOMPLETIONTASK pTask, bool fLocal)
+ */
+static void pdmR3AsyncCompletionPutTask(PPDMASYNCCOMPLETIONENDPOINT pEndpoint, PPDMASYNCCOMPLETIONTASK pTask)
 {
     PPDMASYNCCOMPLETIONEPCLASS pEndpointClass = pEndpoint->pEpClass;
@@ -921 +837 @@
 #endif
 
-    /* Check whether we can use the per endpoint cache */
-    if (   fLocal
-        && (pEndpoint->cTasksCached < pEndpointClass->cEndpointCacheSize))
-    {
-        /* Add it to the list. */
-        pTask->pPrev = NULL;
-        pEndpoint->pTasksFreeTail->pNext = pTask;
-        pEndpoint->pTasksFreeTail        = pTask;
-        ASMAtomicIncU32(&pEndpoint->cTasksCached);
-    }
-    else if (ASMAtomicReadU32(&pEndpoint->cTasksCached) < pEndpointClass->cEpClassCacheSize)
-    {
-        /* Use the global cache. */
-        ASMAtomicIncU32(&pEndpointClass->cTasksCached);
-
-        PPDMASYNCCOMPLETIONTASK pNext;
-        do
-        {
-            pNext = pEndpointClass->apTaskCache[pEndpoint->iSlotStart];
-            pTask->pNext = pNext;
-        } while (!ASMAtomicCmpXchgPtr((void * volatile *)&pEndpointClass->apTaskCache[pEndpoint->iSlotStart], (void *)pTask, (void *)pNext));
-    }
-    else
-    {
-        /* Free it */
-        MMR3HeapFree(pTask);
-    }
+    RTMemCacheFree(pEndpointClass->hMemCacheTasks, pTask);
 }
 
@@ -1005 +895 @@
             pEndpoint->pPrev             = NULL;
             pEndpoint->pEpClass          = pEndpointClass;
-            pEndpoint->pTasksFreeHead    = NULL;
-            pEndpoint->pTasksFreeTail    = NULL;
-            pEndpoint->cTasksCached      = 0;
             pEndpoint->uTaskIdNext       = 0;
             pEndpoint->fTaskIdWraparound = false;
             pEndpoint->pTemplate         = pTemplate;
-            pEndpoint->iSlotStart        = pEndpointClass->cEndpoints % RT_ELEMENTS(pEndpointClass->apTaskCache);
             pEndpoint->pszUri            = RTStrDup(pszFilename);
             pEndpoint->cUsers            = 1;
@@ -1124 +1010 @@
                 && RT_SUCCESS(rc))
             {
-                /* Init the cache. */
-                rc = MMR3HeapAllocZEx(pVM, MM_TAG_PDM_ASYNC_COMPLETION,
-                                      pEndpointClass->pEndpointOps->cbTask,
-                                      (void **)&pEndpoint->pTasksFreeHead);
+                /* Call the initializer for the endpoint. */
+                rc = pEndpointClass->pEndpointOps->pfnEpInitialize(pEndpoint, pszFilename, fFlags);
                 if (RT_SUCCESS(rc))
                 {
-                    pEndpoint->pTasksFreeTail = pEndpoint->pTasksFreeHead;
-
-                    /* Call the initializer for the endpoint. */
-                    rc = pEndpointClass->pEndpointOps->pfnEpInitialize(pEndpoint, pszFilename, fFlags);
-                    if (RT_SUCCESS(rc))
-                    {
-                        /* Link it into the list of endpoints. */
-                        rc = RTCritSectEnter(&pEndpointClass->CritSect);
-                        AssertMsg(RT_SUCCESS(rc), ("Failed to enter critical section rc=%Rrc\n", rc));
-
-                        pEndpoint->pNext = pEndpointClass->pEndpointsHead;
-                        if (pEndpointClass->pEndpointsHead)
-                            pEndpointClass->pEndpointsHead->pPrev = pEndpoint;
-
-                        pEndpointClass->pEndpointsHead = pEndpoint;
-                        pEndpointClass->cEndpoints++;
-
-                        rc = RTCritSectLeave(&pEndpointClass->CritSect);
-                        AssertMsg(RT_SUCCESS(rc), ("Failed to enter critical section rc=%Rrc\n", rc));
-
-                        /* Reference the template. */
-                        ASMAtomicIncU32(&pTemplate->cUsed);
-
-                        *ppEndpoint = pEndpoint;
-
-                        LogFlowFunc((": Created endpoint for %s: rc=%Rrc\n", pszFilename, rc));
-                        return VINF_SUCCESS;
-                    }
-                    MMR3HeapFree(pEndpoint->pTasksFreeHead);
-                    RTStrFree(pEndpoint->pszUri);
+                    /* Link it into the list of endpoints. */
+                    rc = RTCritSectEnter(&pEndpointClass->CritSect);
+                    AssertMsg(RT_SUCCESS(rc), ("Failed to enter critical section rc=%Rrc\n", rc));
+
+                    pEndpoint->pNext = pEndpointClass->pEndpointsHead;
+                    if (pEndpointClass->pEndpointsHead)
+                        pEndpointClass->pEndpointsHead->pPrev = pEndpoint;
+
+                    pEndpointClass->pEndpointsHead = pEndpoint;
+                    pEndpointClass->cEndpoints++;
+
+                    rc = RTCritSectLeave(&pEndpointClass->CritSect);
+                    AssertMsg(RT_SUCCESS(rc), ("Failed to enter critical section rc=%Rrc\n", rc));
+
+                    /* Reference the template. */
+                    ASMAtomicIncU32(&pTemplate->cUsed);
+
+                    *ppEndpoint = pEndpoint;
+
+                    LogFlowFunc((": Created endpoint for %s: rc=%Rrc\n", pszFilename, rc));
+                    return VINF_SUCCESS;
                 }
-                else
-                    rc = VERR_NO_MEMORY;
+                RTStrFree(pEndpoint->pszUri);
             }
             MMR3HeapFree(pEndpoint);
@@ -1195 +1069 @@
         PPDMASYNCCOMPLETIONEPCLASS pEndpointClass = pEndpoint->pEpClass;
         pEndpointClass->pEndpointOps->pfnEpClose(pEndpoint);
-
-        /* Free cached tasks. */
-        PPDMASYNCCOMPLETIONTASK pTask = pEndpoint->pTasksFreeHead;
-
-        while (pTask)
-        {
-            PPDMASYNCCOMPLETIONTASK pTaskFree = pTask;
-            pTask = pTask->pNext;
-            MMR3HeapFree(pTaskFree);
-        }
 
         /* Drop reference from the template. */
@@ -1279 +1143 @@
     }
     else
-        pdmR3AsyncCompletionPutTask(pEndpoint, pTask, false);
+        pdmR3AsyncCompletionPutTask(pEndpoint, pTask);
 
     return rc;
@@ -1311 +1175 @@
     }
     else
-        pdmR3AsyncCompletionPutTask(pEndpoint, pTask, false);
+        pdmR3AsyncCompletionPutTask(pEndpoint, pTask);
 
     return rc;
@@ -1337 +1201 @@
     }
     else
-        pdmR3AsyncCompletionPutTask(pEndpoint, pTask, false);
+        pdmR3AsyncCompletionPutTask(pEndpoint, pTask);
 
     return rc;

trunk/src/VBox/VMM/PDMAsyncCompletionInternal.h

@@ -25 +25 @@
 #include <iprt/cdefs.h>
 #include <iprt/critsect.h>
+#include <iprt/memcache.h>
 #include <VBox/types.h>
 #include <VBox/cfgm.h>
@@ -198 +199 @@
     /** Pointer to the callback table. */
     R3PTRTYPE(PCPDMASYNCCOMPLETIONEPCLASSOPS)   pEndpointOps;
-    /** Bigger cache for free task items used by all endpoints
-     * of this class. */
-    R3PTRTYPE(volatile PPDMASYNCCOMPLETIONTASK) apTaskCache[10];
-    /** Number of tasks cached */
-    volatile uint32_t                           cTasksCached;
+    /** Task cache. */
+    RTMEMCACHE                                  hMemCacheTasks;
 } PDMASYNCCOMPLETIONEPCLASS;
 /** Pointer to the PDM async completion endpoint class data. */
@@ -219 +217 @@
     /** Pointer to the class this endpoint belongs to. */
     R3PTRTYPE(PPDMASYNCCOMPLETIONEPCLASS)       pEpClass;
-    /** Head of the small cache for allocated task structures for exclusive
-     * use by this endpoint. */
-    R3PTRTYPE(volatile PPDMASYNCCOMPLETIONTASK) pTasksFreeHead;
-    /** Tail of the small cache for allocated task structures for exclusive
-     * use by this endpoint. */
-    R3PTRTYPE(volatile PPDMASYNCCOMPLETIONTASK) pTasksFreeTail;
-    /** Number of elements in the cache. */
-    volatile uint32_t                           cTasksCached;
-    /** Start slot for the global task cache. */
-    unsigned                                    iSlotStart;
     /** ID of the next task to ensure consistency. */
     volatile uint32_t                           uTaskIdNext;