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

["Zeng, Star" <star.zeng@intel.com>]

CoreAllocatePoolPages() could not be updated simply by adding CoreAcquireMemoryLock() and CoreReleaseMemoryLock(), it is also used by AllocateMemoryMapEntry() with the lock locked.

Thanks,
Star 
-----Original Message-----
From: Gao, Liming 
Sent: Monday, February 27, 2017 2:43 PM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org
Cc: afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations

Ard:
  I agree to separate lock for pool allocations. I suggest you update CoreAllocatePoolPages() and CoreFreePoolPages() implementation by adding CoreAcquireMemoryLock() and CoreReleaseMemoryLock(). If so, you don't need to introduce new CoreAllocatePoolPagesI () and CoreFreePoolPagesI (). 

Thanks
Liming
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Monday, February 27, 2017 2:30 AM
>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; 
>leif.lindholm@linaro.org
>Cc: afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>; 
>Gao, Liming <liming.gao@intel.com>; lersek@redhat.com; Tian, Feng 
><feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel 
><ard.biesheuvel@linaro.org>
>Subject: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for 
>pool allocations
>
>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 ();
>+  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