Re: [PATCH 2/3] Add Lazy slab initialization

[Paul Durrant <xadimgnik@xxxxxxxxx>]

On 19/07/2021 11:03, Owen Smith wrote:
> Only initialize 1/4 of a slab initially, unless a reservation is defined.
> Allocate and initialize each slab object once this initial allocation has been
> exceeded.
>
> This is to decrease the overall resource usage when each cache object's
> constructor allocates additional memory.
Whilst I understand the motivation, I don't think this is the right 
solution. If the resource consumption is too great to justify the 
performance benefit of using the slab constructor to allocate additional 
memory then the owner of the cache should simply refrain from doing so 
and defer that work until the point it actually allocates the object 
from the cache. There should be no need for a global change such as this.
  Paul

> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> ---
>   src/xenbus/cache.c | 27 +++++++++++++++++++++++++--
>   1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/src/xenbus/cache.c b/src/xenbus/cache.c
> index 247f244..d695ee3 100644
> --- a/src/xenbus/cache.c
> +++ b/src/xenbus/cache.c
> @@ -61,6 +61,7 @@ typedef struct _XENBUS_CACHE_SLAB {
>       LIST_ENTRY      ListEntry;
>       USHORT          MaximumOccupancy;
>       USHORT          CurrentOccupancy;
> +    USHORT          CurrentAllocated;
>       ULONG           *Mask;
>       UCHAR           Buffer[1];
>   } XENBUS_CACHE_SLAB, *PXENBUS_CACHE_SLAB;
> @@ -301,6 +302,7 @@ CacheCreateSlab(
>       ULONG               Size;
>       LONG                Index;
>       LONG                SlabCount;
> +    LONG                ObjectsToAllocate;
>       NTSTATUS            status;
>   
>       NumberOfBytes = P2ROUNDUP(FIELD_OFFSET(XENBUS_CACHE_SLAB, Buffer) +
> @@ -326,6 +328,7 @@ CacheCreateSlab(
>       Slab->Magic = XENBUS_CACHE_SLAB_MAGIC;
>       Slab->Cache = Cache;
>       Slab->MaximumOccupancy = (USHORT)Count;
> +    Slab->CurrentAllocated = 0;
>   
>       Size = P2ROUNDUP(Count, BITS_PER_ULONG);
>       Size /= 8;
> @@ -334,12 +337,18 @@ CacheCreateSlab(
>       if (Slab->Mask == NULL)
>           goto fail3;
>   
> -    for (Index = 0; Index < (LONG)Slab->MaximumOccupancy; Index++) {
> +    ObjectsToAllocate = (Cache->Reservation == 0) ?
> +                (LONG)Count / 4 :
> +                min((LONG)Cache->Reservation, (LONG)Count);
> +
> +    for (Index = 0; Index < ObjectsToAllocate; Index++) {
>           PVOID Object = (PVOID)&Slab->Buffer[Index * Cache->Size];
>   
>           status = __CacheCtor(Cache, Object);
>           if (!NT_SUCCESS(status))
>               goto fail4;
> +
> +        Slab->CurrentAllocated++;
>       }
>   
>       CacheInsertSlab(Cache, Slab);
> @@ -402,7 +411,7 @@ CacheDestroySlab(
>       ASSERT(Cache->Cursor != &Cache->SlabList ||
>              IsListEmpty(&Cache->SlabList));
>   
> -    Index = Slab->MaximumOccupancy;
> +    Index = Slab->CurrentAllocated;
>       while (--Index >= 0) {
>           PVOID Object = (PVOID)&Slab->Buffer[Index * Cache->Size];
>   
> @@ -493,6 +502,20 @@ CacheGetObjectFromSlab(
>       if (Slab->CurrentOccupancy == Slab->MaximumOccupancy)
>           return NULL;
>   
> +    ASSERT3U(Slab->CurrentOccupancy, <=, Slab->CurrentAllocated);
> +    ASSERT3U(Slab->CurrentAllocated, <=, Slab->MaximumOccupancy);
> +    if (Slab->CurrentOccupancy == Slab->CurrentAllocated) {
> +        NTSTATUS            status;
> +
> +        Object = (PVOID)&Slab->Buffer[Slab->CurrentAllocated * Cache->Size];
> +
> +        status = __CacheCtor(Cache, Object);
> +        if (!NT_SUCCESS(status))
> +            return NULL;
> +
> +        Slab->CurrentAllocated++;
> +    }
> +
>       Index = __CacheMaskScan(Slab->Mask, Slab->MaximumOccupancy);
>       BUG_ON(Index >= Slab->MaximumOccupancy);
>