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

[Owen Smith <owen.smith@xxxxxxxxxx>]

-----Original Message-----
From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of 
Paul Durrant
Sent: Monday, July 19, 2021 11:54 AM
To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH 2/3] Add Lazy slab initialization

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

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


Its possible that this is not need when the 3rd patch (Remove 
MINIMUM_OBJECT_SIZE) is used. 
In XenVbd, the bounce buffers had a reservation of 32 objects (each object will 
allocate a page of memory). This unfortunately currently results in 2 slabs of 
31 objects, which is far more than used in a relatively unused VM 
(experimentally, 16 bounce buffers are used at any one time).
This does leave the possibility of consuming significant memory when a new slab 
is required. At worse this can be (PAGE_SIZE - sizeof(XENBUS_CACHE_SLAB)) / 
sizeof(PVOID) objects allocating a single page, or less objects allocating 
multiple pages.

I will look at making users of the cache interface attempt to avoid 
preallocation of pages during the constructor call.

Owen

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