1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time.

Dong, Eric posted 1 patch 4 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/edk2 tags/patchew/20191128025814.417-1-eric.dong@intel.com
There is a newer version of this series
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      | 56 +++++++++++++++++++---
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 16 +++++++
2 files changed, 66 insertions(+), 6 deletions(-)
[edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time.
Posted by Dong, Eric 4 years, 4 months ago 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2388

Current logic allocate Token every time when need to use it. The logic
caused SMI latency raised to very high. Update logic to allocate Token
buffer at driver's entry point. Later use the token from the allocated
token buffer. Only when all the buffer have been used, then need to
allocate new buffer.

Signed-off-by: Eric Dong <eric.dong@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      | 56 +++++++++++++++++++---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 16 +++++++
 2 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index d8d2b6f444..702bbcd095 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -492,6 +492,24 @@ FreeTokens (
 {
   LIST_ENTRY            *Link;
   PROCEDURE_TOKEN       *ProcToken;
+  TOKEN_BUFFER          *TokenBuf;
+
+  //
+  // Only reset current used Token buffer, not free this buffer.
+  //
+  gSmmCpuPrivate->UsedTokenNum = 0;
+
+  Link = GetFirstNode (&gSmmCpuPrivate->OldTokenBufList);
+  while (!IsNull (&gSmmCpuPrivate->OldTokenBufList, Link)) {
+    TokenBuf = TOKEN_BUFFER_FROM_LINK (Link);
+
+    Link = GetNextNode (&gSmmCpuPrivate->OldTokenBufList, Link);
+
+    RemoveEntryList (&TokenBuf->Link);
+    
+    FreePool (TokenBuf->Buffer);
+    FreePool (TokenBuf);
+  }
 
   while (!IsListEmpty (&gSmmCpuPrivate->TokenList)) {
     Link = GetFirstNode (&gSmmCpuPrivate->TokenList);
@@ -499,7 +517,6 @@ FreeTokens (
 
     RemoveEntryList (&ProcToken->Link);
 
-    FreePool ((VOID *)ProcToken->ProcedureToken);
     FreePool (ProcToken);
   }
 }
@@ -1115,13 +1132,32 @@ CreateToken (
   VOID
   )
 {
-  PROCEDURE_TOKEN    *ProcToken;
+  PROCEDURE_TOKEN     *ProcToken;
   SPIN_LOCK           *CpuToken;
-  UINTN               SpinLockSize;
+  TOKEN_BUFFER        *TokenBuf;
+
+  if (gSmmCpuPrivate->UsedTokenNum >= MAX_TOKEN_COUND_NUMBER - 1) {
+    DEBUG((DEBUG_INFO, "Token buffer not enough, allocate new buffer[TokenNum=0x%x]!\n", gSmmCpuPrivate->UsedTokenNum));
+
+    //
+    // Save Current Token Buffer to the list.
+    //
+    TokenBuf = AllocatePool (sizeof (TOKEN_BUFFER));
+    ASSERT (TokenBuf != NULL);
+    TokenBuf->Signature = TOKEN_BUFFER_SIGNATURE;
+    TokenBuf->Buffer  = gSmmCpuPrivate->CurrentTokenBuf;
+
+    InsertTailList (&gSmmCpuPrivate->OldTokenBufList, &TokenBuf->Link);
+
+    gSmmCpuPrivate->CurrentTokenBuf   = AllocateZeroPool (gSmmCpuPrivate->TokenSize * MAX_TOKEN_COUND_NUMBER);
+    ASSERT (gSmmCpuPrivate->CurrentTokenBuf != NULL);
+    gSmmCpuPrivate->UsedTokenNum = 0;
+  }
+
+  CpuToken = (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + gSmmCpuPrivate->TokenSize * gSmmCpuPrivate->UsedTokenNum);
+  gSmmCpuPrivate->UsedTokenNum++;
+  //DEBUG((DEBUG_INFO, "Token Address = 0x%x, Token Number = 0x%x\n", CpuToken, gSmmCpuPrivate->UsedTokenNum));
 
-  SpinLockSize = GetSpinLockProperties ();
-  CpuToken = AllocatePool (SpinLockSize);
-  ASSERT (CpuToken != NULL);
   InitializeSpinLock (CpuToken);
   AcquireSpinLock (CpuToken);
 
@@ -1737,10 +1773,18 @@ InitializeDataForMmMp (
   VOID
   )
 {
+  gSmmCpuPrivate->TokenSize = (UINT32)GetSpinLockProperties ();
+  DEBUG((DEBUG_INFO, "gSmmCpuPrivate->TokenSize = 0x%x\n", gSmmCpuPrivate->TokenSize));
+
+  gSmmCpuPrivate->CurrentTokenBuf   = AllocateZeroPool (gSmmCpuPrivate->TokenSize * MAX_TOKEN_COUND_NUMBER);
+  ASSERT (gSmmCpuPrivate->CurrentTokenBuf != NULL);
+  gSmmCpuPrivate->UsedTokenNum = 0;
+
   gSmmCpuPrivate->ApWrapperFunc = AllocatePool (sizeof (PROCEDURE_WRAPPER) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus);
   ASSERT (gSmmCpuPrivate->ApWrapperFunc != NULL);
 
   InitializeListHead (&gSmmCpuPrivate->TokenList);
+  InitializeListHead (&gSmmCpuPrivate->OldTokenBufList);
 }
 
 /**
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 8c29f1a558..c197aad288 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -198,6 +198,7 @@ typedef UINT32                              SMM_CPU_ARRIVAL_EXCEPTIONS;
 #define ARRIVAL_EXCEPTION_DELAYED           0x2
 #define ARRIVAL_EXCEPTION_SMI_DISABLED      0x4
 
+#define MAX_TOKEN_COUND_NUMBER              0x512
 //
 // Wrapper used to convert EFI_AP_PROCEDURE2 and EFI_AP_PROCEDURE.
 //
@@ -217,6 +218,17 @@ typedef struct {
 
 #define PROCEDURE_TOKEN_FROM_LINK(a)  CR (a, PROCEDURE_TOKEN, Link, PROCEDURE_TOKEN_SIGNATURE)
 
+#define TOKEN_BUFFER_SIGNATURE  SIGNATURE_32 ('T', 'K', 'B', 'S')
+
+typedef struct {
+  UINTN                   Signature;
+  LIST_ENTRY              Link;
+
+  UINT8                   *Buffer;
+} TOKEN_BUFFER;
+
+#define TOKEN_BUFFER_FROM_LINK(a)  CR (a, TOKEN_BUFFER, Link, TOKEN_BUFFER_SIGNATURE)
+
 //
 // Private structure for the SMM CPU module that is stored in DXE Runtime memory
 // Contains the SMM Configuration Protocols that is produced.
@@ -243,6 +255,10 @@ typedef struct {
   PROCEDURE_WRAPPER               *ApWrapperFunc;
   LIST_ENTRY                      TokenList;
 
+  LIST_ENTRY                      OldTokenBufList;
+  UINT8                           *CurrentTokenBuf;
+  UINTN                           UsedTokenNum;     // Only record tokens used in CurrentTokenBuf.
+  UINT32                          TokenSize;
 } SMM_CPU_PRIVATE_DATA;
 
 extern SMM_CPU_PRIVATE_DATA  *gSmmCpuPrivate;
-- 
2.23.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#51437): https://edk2.groups.io/g/devel/message/51437
Mute This Topic: https://groups.io/mt/63641247/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time.
Posted by Ni, Ray 4 years, 4 months ago 

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dong,
> Eric
> Sent: Thursday, November 28, 2019 10:58 AM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid
> allocate Token every time.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2388
> 
> Current logic allocate Token every time when need to use it. The logic
> caused SMI latency raised to very high. Update logic to allocate Token
> buffer at driver's entry point. Later use the token from the allocated
> token buffer. Only when all the buffer have been used, then need to
> allocate new buffer.
> 
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> 
> Cc: Eric Dong <eric.dong@intel.com>
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      | 56
> +++++++++++++++++++---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 16 +++++++
>  2 files changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index d8d2b6f444..702bbcd095 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -492,6 +492,24 @@ FreeTokens (
>  {
> 
>    LIST_ENTRY            *Link;
> 
>    PROCEDURE_TOKEN       *ProcToken;
> 
> +  TOKEN_BUFFER          *TokenBuf;
> 
> +
> 
> +  //
> 
> +  // Only reset current used Token buffer, not free this buffer.
> 
> +  //
> 
> +  gSmmCpuPrivate->UsedTokenNum = 0;
> 
> +
> 
> +  Link = GetFirstNode (&gSmmCpuPrivate->OldTokenBufList);
> 
> +  while (!IsNull (&gSmmCpuPrivate->OldTokenBufList, Link)) {
> 
> +    TokenBuf = TOKEN_BUFFER_FROM_LINK (Link);
> 
> +
> 
> +    Link = GetNextNode (&gSmmCpuPrivate->OldTokenBufList, Link);
> 
> +
> 
> +    RemoveEntryList (&TokenBuf->Link);

1. The above two lines can be combined to below single line:
Link = RemoveEntryList (&TokenBuf->Link);
> 
> +
> 
> +    FreePool (TokenBuf->Buffer);
> 
> +    FreePool (TokenBuf);
> 
> +  }
> 
> 
> 
>    while (!IsListEmpty (&gSmmCpuPrivate->TokenList)) {
> 
>      Link = GetFirstNode (&gSmmCpuPrivate->TokenList);
> 
> @@ -499,7 +517,6 @@ FreeTokens (
> 
> 
>      RemoveEntryList (&ProcToken->Link);
> 
> 
> 
> -    FreePool ((VOID *)ProcToken->ProcedureToken);
> 
>      FreePool (ProcToken);
> 
>    }
> 
>  }
> 
> @@ -1115,13 +1132,32 @@ CreateToken (
>    VOID
> 
>    )
> 
>  {
> 
> -  PROCEDURE_TOKEN    *ProcToken;
> 
> +  PROCEDURE_TOKEN     *ProcToken;
> 
>    SPIN_LOCK           *CpuToken;
> 
> -  UINTN               SpinLockSize;
> 
> +  TOKEN_BUFFER        *TokenBuf;
> 
> +
> 
> +  if (gSmmCpuPrivate->UsedTokenNum >= MAX_TOKEN_COUND_NUMBER
> - 1) {

2. Can the macro be MAX_TOKEN_COUNT? The variable be " gSmmCpuPrivate->TokenCount"?
Usually when we define a storage, there are three variables: Elements[], Count, Capacity.
MAX_TOKEN_COUNT is the capacity. TokenCount is the count.

3. should be "TokenCount == MAX_TOKEN_COUNT".

4. Can you explain how the toke buffer is managed using comments?

> 
> +    DEBUG((DEBUG_INFO, "Token buffer not enough, allocate new
> buffer[TokenNum=0x%x]!\n", gSmmCpuPrivate->UsedTokenNum));

5. Suggest add prefix like "CpuSmm: ".

> 
> +
> 
> +    //
> 
> +    // Save Current Token Buffer to the list.
> 
> +    //
> 
> +    TokenBuf = AllocatePool (sizeof (TOKEN_BUFFER));
> 
> +    ASSERT (TokenBuf != NULL);
> 
> +    TokenBuf->Signature = TOKEN_BUFFER_SIGNATURE;
> 
> +    TokenBuf->Buffer  = gSmmCpuPrivate->CurrentTokenBuf;
> 
> +
> 
> +    InsertTailList (&gSmmCpuPrivate->OldTokenBufList, &TokenBuf->Link);
> 
> +
> 
> +    gSmmCpuPrivate->CurrentTokenBuf   = AllocateZeroPool
> (gSmmCpuPrivate->TokenSize * MAX_TOKEN_COUND_NUMBER);
> 
> +    ASSERT (gSmmCpuPrivate->CurrentTokenBuf != NULL);
> 
> +    gSmmCpuPrivate->UsedTokenNum = 0;
> 
> +  }
> 
> +
> 
> +  CpuToken = (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf +
> gSmmCpuPrivate->TokenSize * gSmmCpuPrivate->UsedTokenNum);
> 
> +  gSmmCpuPrivate->UsedTokenNum++;
> 
> +  //DEBUG((DEBUG_INFO, "Token Address = 0x%x, Token Number =
> 0x%x\n", CpuToken, gSmmCpuPrivate->UsedTokenNum));
> 
> 
> 
> -  SpinLockSize = GetSpinLockProperties ();
> 
> -  CpuToken = AllocatePool (SpinLockSize);
> 
> -  ASSERT (CpuToken != NULL);
> 
>    InitializeSpinLock (CpuToken);
> 
>    AcquireSpinLock (CpuToken);
> 
> 
> 
> @@ -1737,10 +1773,18 @@ InitializeDataForMmMp (
>    VOID
> 
>    )
> 
>  {
> 
> +  gSmmCpuPrivate->TokenSize = (UINT32)GetSpinLockProperties ();

6. How about not to cache the TokenSize?

> 
> +  DEBUG((DEBUG_INFO, "gSmmCpuPrivate->TokenSize = 0x%x\n",
> gSmmCpuPrivate->TokenSize));

7. Maybe no need.

> 
> +
> 
> +  gSmmCpuPrivate->CurrentTokenBuf   = AllocateZeroPool
> (gSmmCpuPrivate->TokenSize * MAX_TOKEN_COUND_NUMBER);
> 
> +  ASSERT (gSmmCpuPrivate->CurrentTokenBuf != NULL);
> 
> +  gSmmCpuPrivate->UsedTokenNum = 0;
> 
> +
> 
>    gSmmCpuPrivate->ApWrapperFunc = AllocatePool (sizeof
> (PROCEDURE_WRAPPER) * gSmmCpuPrivate-
> >SmmCoreEntryContext.NumberOfCpus);
> 
>    ASSERT (gSmmCpuPrivate->ApWrapperFunc != NULL);
> 
> 
> 
>    InitializeListHead (&gSmmCpuPrivate->TokenList);
> 
> +  InitializeListHead (&gSmmCpuPrivate->OldTokenBufList);
> 
>  }
> 
> 
> 
>  /**
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 8c29f1a558..c197aad288 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -198,6 +198,7 @@ typedef UINT32
> SMM_CPU_ARRIVAL_EXCEPTIONS;
>  #define ARRIVAL_EXCEPTION_DELAYED           0x2
> 
>  #define ARRIVAL_EXCEPTION_SMI_DISABLED      0x4
> 
> 
> 
> +#define MAX_TOKEN_COUND_NUMBER              0x512
> 
>  //
> 
>  // Wrapper used to convert EFI_AP_PROCEDURE2 and EFI_AP_PROCEDURE.
> 
>  //
> 
> @@ -217,6 +218,17 @@ typedef struct {
> 
> 
>  #define PROCEDURE_TOKEN_FROM_LINK(a)  CR (a, PROCEDURE_TOKEN,
> Link, PROCEDURE_TOKEN_SIGNATURE)
> 
> 
> 
> +#define TOKEN_BUFFER_SIGNATURE  SIGNATURE_32 ('T', 'K', 'B', 'S')
> 
> +
> 
> +typedef struct {
> 
> +  UINTN                   Signature;
> 
> +  LIST_ENTRY              Link;
> 
> +
> 
> +  UINT8                   *Buffer;
> 
> +} TOKEN_BUFFER;
> 
> +
> 
> +#define TOKEN_BUFFER_FROM_LINK(a)  CR (a, TOKEN_BUFFER, Link,
> TOKEN_BUFFER_SIGNATURE)
> 
> +
> 
>  //
> 
>  // Private structure for the SMM CPU module that is stored in DXE Runtime
> memory
> 
>  // Contains the SMM Configuration Protocols that is produced.
> 
> @@ -243,6 +255,10 @@ typedef struct {
>    PROCEDURE_WRAPPER               *ApWrapperFunc;
> 
>    LIST_ENTRY                      TokenList;
> 
> 
> 
> +  LIST_ENTRY                      OldTokenBufList;
> 
> +  UINT8                           *CurrentTokenBuf;
> 
> +  UINTN                           UsedTokenNum;     // Only record tokens used in
> CurrentTokenBuf.
> 
> +  UINT32                          TokenSize;
> 
>  } SMM_CPU_PRIVATE_DATA;
> 
> 
> 
>  extern SMM_CPU_PRIVATE_DATA  *gSmmCpuPrivate;
> 
> --
> 2.23.0.windows.1
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#51437): https://edk2.groups.io/g/devel/message/51437
> Mute This Topic: https://groups.io/mt/63641247/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [ray.ni@intel.com]
> -=-=-=-=-=-=


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#51438): https://edk2.groups.io/g/devel/message/51438
Mute This Topic: https://groups.io/mt/63641247/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.