Re: [PATCH 3/3] Allow removal of current Slab

[Owen Smith <owen.smith@xxxxxxxxx>]

     if (Cache->Cursor == &Slab->ListEntry)
-        Cache->Cursor = &Cache->SlabList;
+        Cache->Cursor = Slab->ListEntry.Blink;

Looks like this blind walk back could end up with Cursor pointing to a full Slab.

Thinking about it, a more elegant solution could be to not remove the empty Slab if its the current slab (removes the need to destroy an empty slab, probably shortly followed by adding a new Slab when the nex CacheGet is called). The teardown case where CacheSpill(Cache, 0), on the CacheCreate fail path and CacheDestroy should NULL the Cursor, effectively ignoring the Cursor == &SlabList check

Let me try a patch that implements this method

Owen

On Wed, Jun 7, 2023 at 1:02 PM Durrant, Paul <xadimgnik@xxxxxxxxx> wrote:

On 07/06/2023 12:53, Owen Smith wrote:
> Then it looks like a minor reordering is needed. The check for
> CacheMaskCount(Slab) == CacheMaskSize(Slab) should be made before the
> attempt to call CacheGetObjectFromSlab(Slab), as if Slab is full,
> CacheGetObjectFromSlab will fail and return NULL, triggering the ASSERT
> i.e.
> if (Cache->Cursor != &Cache->SlabList) {
>    PLIST_ENTRY ListEntry = Cache->Cursor;
>    PXENBUS_CACHE_SLAB Slab;
>    Slab = CONTAINING_RECORD(ListEntry, XENBUS_CACHE_SLAB, ListEntry);
>    if (CacheMaskCount(Slab->Allocated) == CacheMaskSize(Slab->Allocated)) {
>      // this slab is full, try next slab
>      Cache->Cursor = Slab->ListEntry.Flink;
>      goto again;
>    }
>    // there are free objects in this slab, this should always work
>    Object = CacheGetObjectFromSlab(Slab);
>    ASSERT(Object != NULL);
> }
>
[snip]

The question is why can we end up with Cache->Cursor !=
&Cache->SlabList, but yet the slab is full? This is not a valid state
for the cache to be in.

   Paul