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

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

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