Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 26 February 2017 at 18:30, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> In preparation of adding memory permission attribute management to
> the pool allocator, split off the locking of the pool metadata into
> a separate lock. This is an improvement in itself, given that pool
> allocation can only interfere with the page allocation bookkeeping
> if pool pages are allocated or released. But it is also required to
> ensure that the permission attribute management does not deadlock,
> given that it may trigger page table splits leading to additional
> page tables being allocated.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> index 7afd2d312c1d..410615e0dee9 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> @@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include "DxeMain.h"
>  #include "Imem.h"
>
> +STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_NOTIFY);
> +
>  #define POOL_FREE_SIGNATURE   SIGNATURE_32('p','f','r','0')
>  typedef struct {
>    UINT32          Signature;
> @@ -239,13 +241,13 @@ CoreInternalAllocatePool (
>    //
>    // Acquire the memory lock and make the allocation
>    //
> -  Status = CoreAcquireLockOrFail (&gMemoryLock);
> +  Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
>    if (EFI_ERROR (Status)) {
>      return EFI_OUT_OF_RESOURCES;
>    }
>
>    *Buffer = CoreAllocatePoolI (PoolType, Size);
> -  CoreReleaseMemoryLock ();
> +  CoreReleaseLock (&mPoolMemoryLock);
>    return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES;
>  }
>
> @@ -289,6 +291,23 @@ CoreAllocatePool (
>    return Status;
>  }
>
> +STATIC
> +VOID *
> +CoreAllocatePoolPagesI (
> +  IN EFI_MEMORY_TYPE    PoolType,
> +  IN UINTN              NoPages,
> +  IN UINTN              Granularity
> +  )
> +{
> +  VOID    *Buffer;
> +
> +  CoreAcquireMemoryLock ();

This should probably be

  EFI_STATUS  Status;

  Status = CoreAcquireLockOrFail (&gMemoryLock);
  if (EFI_ERROR (Status)) {
    return NULL;
  }

to preserve the old behavior.

> +  Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
> +  CoreReleaseMemoryLock ();
> +
> +  return Buffer;
> +}
> +
>  /**
>    Internal function to allocate pool of a particular type.
>    Caller must have the memory lock held
> @@ -317,7 +336,7 @@ CoreAllocatePoolI (
>    UINTN       NoPages;
>    UINTN       Granularity;
>
> -  ASSERT_LOCKED (&gMemoryLock);
> +  ASSERT_LOCKED (&mPoolMemoryLock);
>
>    if  (PoolType == EfiACPIReclaimMemory   ||
>         PoolType == EfiACPIMemoryNVS       ||
> @@ -355,7 +374,7 @@ CoreAllocatePoolI (
>    if (Index >= SIZE_TO_LIST (Granularity)) {
>      NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
>      NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
> -    Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
> +    Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
>      goto Done;
>    }
>
> @@ -383,7 +402,7 @@ CoreAllocatePoolI (
>      //
>      // Get another page
>      //
> -    NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
> +    NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
>      if (NewPage == NULL) {
>        goto Done;
>      }
> @@ -486,9 +505,9 @@ CoreInternalFreePool (
>      return EFI_INVALID_PARAMETER;
>    }
>
> -  CoreAcquireMemoryLock ();
> +  CoreAcquireLock (&mPoolMemoryLock);
>    Status = CoreFreePoolI (Buffer, PoolType);
> -  CoreReleaseMemoryLock ();
> +  CoreReleaseLock (&mPoolMemoryLock);
>    return Status;
>  }
>
> @@ -525,6 +544,19 @@ CoreFreePool (
>    return Status;
>  }
>
> +STATIC
> +VOID
> +CoreFreePoolPagesI (
> +  IN EFI_MEMORY_TYPE        PoolType,
> +  IN EFI_PHYSICAL_ADDRESS   Memory,
> +  IN UINTN                  NoPages
> +  )
> +{
> +  CoreAcquireMemoryLock ();
> +  CoreFreePoolPages (Memory, NoPages);
> +  CoreReleaseMemoryLock ();
> +}
> +
>  /**
>    Internal function to free a pool entry.
>    Caller must have the memory lock held
> @@ -573,7 +605,7 @@ CoreFreePoolI (
>    //
>    ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
>    ASSERT (Head->Size == Tail->Size);
> -  ASSERT_LOCKED (&gMemoryLock);
> +  ASSERT_LOCKED (&mPoolMemoryLock);
>
>    if (Tail->Signature != POOL_TAIL_SIGNATURE) {
>      return EFI_INVALID_PARAMETER;
> @@ -624,7 +656,7 @@ CoreFreePoolI (
>      //
>      NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
>      NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
> -    CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
> +    CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
>
>    } else {
>
> @@ -680,7 +712,8 @@ CoreFreePoolI (
>          //
>          // Free the page
>          //
> -        CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage, EFI_SIZE_TO_PAGES (Granularity));
> +        CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
> +          EFI_SIZE_TO_PAGES (Granularity));
>        }
>      }
>    }
> --
> 2.7.4
>