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

[Owen Smith <owen.smith@xxxxxxxxx>]

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);

}

Owen

On Wed, Jun 7, 2023 at 10:51 AM Durrant, Paul <xadimgnik@xxxxxxxxx> wrote:

[CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.

On 25/05/2023 16:14, Owen Smith wrote:
> From: Owen Smith <owen.smith@xxxxxxxxxx>
>
> When all Objects in the current Slab have been freed, its possible
> that the current Slab will be destroyed. Its not guaranteed that the
> current Slab is the only Slab in the SlabList, as objects can be returned
> in a different order than allocated, especially after objects have been
> reused.
>
> Walk the Cursor back to the previous Slab (or SlabList header)

Yes, this looks like something that was missed when CacheSpill() was
added. I'm happy to take this part.

> and allow
> for the possibility that the current Slab may be full during the CacheGet
> call (which will require creation of another Slab)
>

There's an ASSERT to check that the cursor slab is not full so I don't
think your modification to CacheGet() is correct.
The code is:

  610    if (Cache->Cursor != &Cache->SlabList) {
  611        PLIST_ENTRY ListEntry = Cache->Cursor;
  612        PXENBUS_CACHE_SLAB  Slab;
  613
  614        Slab = CONTAINING_RECORD(ListEntry, XENBUS_CACHE_SLAB,
ListEntry);
  615
  616        Object = CacheGetObjectFromSlab(Slab);
  617        ASSERT(Object != NULL);
  618
  619        if (CacheMaskCount(Slab->Allocated) ==
CacheMaskSize(Slab->Allocated))
  620            Cache->Cursor = Slab->ListEntry.Flink;
  621    }

So if the cursor slab is full it is walked forward, so if there are no
more slabs it will end up pointing at Cache->SlabList, so the ASSERT you
are removing *should* be valid. If you are hitting it then something
else is wrong.

   Paul

> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> ---
>   src/xenbus/cache.c | 17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/src/xenbus/cache.c b/src/xenbus/cache.c
> index 07dcd56..e2aa137 100644
> --- a/src/xenbus/cache.c
> +++ b/src/xenbus/cache.c
> @@ -471,14 +471,11 @@ CacheDestroySlab(
>       ASSERT3U(Cache->Count, >=, CacheMaskSize(Slab->Allocated));
>       Cache->Count -= CacheMaskSize(Slab->Allocated);
>   
> -    //
> -    // The only reason the cursor should be pointing at this slab is
> -    // if it is the only one in the list.
> -    //
>       if (Cache->Cursor == &Slab->ListEntry)
> -        Cache->Cursor = &Cache->SlabList;
> +        Cache->Cursor = Slab->ListEntry.Blink;
>   
> -    RemoveEntryList(&Slab->ListEntry);
> +    if (RemoveEntryList(&Slab->ListEntry))
> +        Cache->Cursor = &Cache->SlabList;
>   
>       ASSERT(Cache->Cursor != &Cache->SlabList ||
>              IsListEmpty(&Cache->SlabList));
> @@ -614,17 +611,17 @@ again:
>           Slab = CONTAINING_RECORD(ListEntry, XENBUS_CACHE_SLAB, ListEntry);
>   
>           Object = CacheGetObjectFromSlab(Slab);
> -        ASSERT(Object != NULL);
>   
> -        if (CacheMaskCount(Slab->Allocated) == CacheMaskSize(Slab->Allocated))
> +        if (CacheMaskCount(Slab->Allocated) == CacheMaskSize(Slab->Allocated)) {
>               Cache->Cursor = Slab->ListEntry.Flink;
> +            if (Object == NULL)
> +                goto again;
> +        }
>       }
>   
>       if (Object == NULL) {
>           NTSTATUS status;
>   
> -        ASSERT3P(Cache->Cursor, ==, &Cache->SlabList);
> -
>           status = CacheCreateSlab(Cache);
>           if (NT_SUCCESS(status)) {
>               ASSERT(Cache->Cursor != &Cache->SlabList);