[edk2,v4,4/7] 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 | 58 ++++++++++++++++----
 1 file changed, 48 insertions(+), 10 deletions(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reviewed-by: Liming Gao <liming.gao@intel.com>


-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 

Sent: Monday, February 27, 2017 10:38 PM
To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Cc: lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: [PATCH v4 4/7] 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 | 58 ++++++++++++++++----
 1 file changed, 48 insertions(+), 10 deletions(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-develdiff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
index 7afd2d312c1d..ebb2fceedd80 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,28 @@ CoreAllocatePool (
   return Status;
 }
 
+STATIC
+VOID *
+CoreAllocatePoolPagesI (
+  IN EFI_MEMORY_TYPE    PoolType,
+  IN UINTN              NoPages,
+  IN UINTN              Granularity
+  )
+{
+  VOID        *Buffer;
+  EFI_STATUS  Status;
+
+  Status = CoreAcquireLockOrFail (&gMemoryLock);
+  if (EFI_ERROR (Status)) {
+    return NULL;
+  }
+
+  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 +341,7 @@ CoreAllocatePoolI (
   UINTN       NoPages;
   UINTN       Granularity;
 
-  ASSERT_LOCKED (&gMemoryLock);
+  ASSERT_LOCKED (&mPoolMemoryLock);
 
   if  (PoolType == EfiACPIReclaimMemory   ||
        PoolType == EfiACPIMemoryNVS       ||
@@ -355,7 +379,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 +407,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 +510,9 @@ CoreInternalFreePool (
     return EFI_INVALID_PARAMETER;
   }
 
-  CoreAcquireMemoryLock ();
+  CoreAcquireLock (&mPoolMemoryLock);
   Status = CoreFreePoolI (Buffer, PoolType);
-  CoreReleaseMemoryLock ();
+  CoreReleaseLock (&mPoolMemoryLock);
   return Status;
 }
 
@@ -525,6 +549,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 +610,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 +661,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 +717,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));
       }
     }
   }