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

["Durrant, Paul" <xadimgnik@xxxxxxxxx>]

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