[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Cache->Cursor may point to Slab


  • To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
  • From: "Durrant, Paul" <xadimgnik@xxxxxxxxx>
  • Date: Mon, 20 May 2024 09:32:26 +0100
  • Delivery-date: Mon, 20 May 2024 08:32:31 +0000
  • List-id: Developer list for the Windows PV Drivers subproject <win-pv-devel.lists.xenproject.org>

On 20/05/2024 09:21, Durrant, Paul wrote:
On 03/05/2024 11:33, Owen Smith wrote:
Its possible that the Cursor refers to the only Slab that has no
allocated objects, but there are other slabs which are fully
allocated. The current ASSERTions are not valid, and needs rework.

Also adds more comments and ASSERTions to validate whats going on.

Signed-off-by: Owen Smith <owen.smith@xxxxxxxxx>
---
  src/xenbus/cache.c | 16 +++++++++++-----
  1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/xenbus/cache.c b/src/xenbus/cache.c
index 6229aed..821f66a 100644
--- a/src/xenbus/cache.c
+++ b/src/xenbus/cache.c
@@ -479,20 +479,25 @@ CacheDestroySlab(
      //
      // The only reason the cursor should be pointing at this slab is
-    // if it is the only one in the list.
+    // if it is the only one in the list with no allocated objects.

Now that we don't spill empty slabs immediately, I think the cursor could actually point at any one of several empty slabs.

      //
      if (Cache->Cursor == &Slab->ListEntry) {
          ASSERT3P(Slab->ListEntry.Flink, ==, &Cache->SlabList);
-        ASSERT3P(Slab->ListEntry.Blink, ==, &Cache->SlabList);
+        // Slab->ListEntry.Blink could refer to a fully allocated Slab
+        // Set Cursor to SlabList, so next CacheGet allocates a new Slab
          Cache->Cursor = &Cache->SlabList;
      }
      RemoveEntryList(&Slab->ListEntry);
+    // If Cache->Cursor == &Cache->SlabList,
+    //     either; all entries in SlabList are fully allocated; or
+    //             SlabList is empty
+    // If Cache->Cursor != &Cache->SlabList,
+    //     Cache->Cursor is the first Slab that is not fully allocated
-    ASSERT(IMPLY(Cache->Cursor == &Cache->SlabList,
-                 IsListEmpty(&Cache->SlabList)));
+    CacheAudit(Cache);

Actually, I think this might cause a problem. The cursor is reset to the anchor above but the audit could find a slab in the list that is not fully occupied, I think.

-    Index = CacheMaskSize(Slab->Allocated);
+    Index = CacheMaskSize(Slab->Constructed);
      while (--Index >= 0) {
          PVOID Object = (PVOID)&Slab->Buffer[Index * Cache->Size];
@@ -501,6 +506,7 @@ CacheDestroySlab(
              __CacheMaskClear(Slab->Constructed, Index);
          }
      }
+    ASSERT3U(CacheMaskCount(Slab->Constructed), ==, 0);
      ASSERT(Cache->CurrentSlabs != 0);
      InterlockedDecrement(&Cache->CurrentSlabs);

Acked-by: Paul Durrant <paul@xxxxxxx>

I'll withdraw that and take a closer look at this.

  Paul





 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.