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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Eric,

On 11/28/19 7:17 AM, Dong, Eric via Groups.Io wrote:
> v2 changes:
>    Minor update based on comments.
> 
> v1 changes:

Nitpick, previous comments should not go into the commit description, 
either on a cover letter, or after the '---' marker so they get stripped 
out by git-am.

Another nitpick, remove the trailing dot in commit subject.

> 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: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>   UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      | 56 ++++++++++++++++++++--
>   UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 16 +++++++
>   2 files changed, 68 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index d8d2b6f444..4632e5b0c2 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -492,6 +492,23 @@ FreeTokens (
>   {
>     LIST_ENTRY            *Link;
>     PROCEDURE_TOKEN       *ProcToken;
> +  TOKEN_BUFFER          *TokenBuf;
> +
> +  //
> +  // Not free the buffer, just clear the UsedTokenNum. In order to
> +  // avoid later trig allcate action again when need to use token.

Maybe: "In order to avoid triggering allocate action when we need to use 
the token, do not free the buffer."

Patch looks good otherwise.

> +  //
> +  gSmmCpuPrivate->UsedTokenNum = 0;
> +
> +  Link = GetFirstNode (&gSmmCpuPrivate->OldTokenBufList);
> +  while (!IsNull (&gSmmCpuPrivate->OldTokenBufList, Link)) {
> +    TokenBuf = TOKEN_BUFFER_FROM_LINK (Link);
> +
> +    Link = RemoveEntryList (&TokenBuf->Link);
> +
> +    FreePool (TokenBuf->Buffer);
> +    FreePool (TokenBuf);
> +  }
>   
>     while (!IsListEmpty (&gSmmCpuPrivate->TokenList)) {
>       Link = GetFirstNode (&gSmmCpuPrivate->TokenList);
> @@ -499,7 +516,6 @@ FreeTokens (
>   
>       RemoveEntryList (&ProcToken->Link);
>   
> -    FreePool ((VOID *)ProcToken->ProcedureToken);
>       FreePool (ProcToken);
>     }
>   }
> @@ -1115,13 +1131,35 @@ CreateToken (
>     VOID
>     )
>   {
> -  PROCEDURE_TOKEN    *ProcToken;
> +  PROCEDURE_TOKEN     *ProcToken;
>     SPIN_LOCK           *CpuToken;
>     UINTN               SpinLockSize;
> +  TOKEN_BUFFER        *TokenBuf;
>   
>     SpinLockSize = GetSpinLockProperties ();
> -  CpuToken = AllocatePool (SpinLockSize);
> -  ASSERT (CpuToken != NULL);
> +
> +  if (gSmmCpuPrivate->UsedTokenNum == MAX_PRE_RESERVE_TOKEN_COUNT) {
> +    DEBUG((DEBUG_INFO, "CpuSmm: No free token buffer, allocate new buffer!\n"));
> +
> +    //
> +    // Record current token buffer for later free action usage.
> +    // Current used token buffer not in this 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 (SpinLockSize * MAX_PRE_RESERVE_TOKEN_COUNT);
> +    ASSERT (gSmmCpuPrivate->CurrentTokenBuf != NULL);
> +    gSmmCpuPrivate->UsedTokenNum = 0;
> +  }
> +
> +  CpuToken = (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + SpinLockSize * gSmmCpuPrivate->UsedTokenNum);
> +  gSmmCpuPrivate->UsedTokenNum++;
> +
>     InitializeSpinLock (CpuToken);
>     AcquireSpinLock (CpuToken);
>   
> @@ -1737,10 +1775,20 @@ InitializeDataForMmMp (
>     VOID
>     )
>   {
> +  UINTN              SpinLockSize;
> +
> +  SpinLockSize = GetSpinLockProperties ();
> +  DEBUG((DEBUG_INFO, "CpuSmm: SpinLock Size = 0x%x\n", SpinLockSize));
> +
> +  gSmmCpuPrivate->CurrentTokenBuf = AllocateZeroPool (SpinLockSize * MAX_PRE_RESERVE_TOKEN_COUNT);
> +  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..36fd0dae92 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_PRE_RESERVE_TOKEN_COUNT                     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.
>   } SMM_CPU_PRIVATE_DATA;
>   
>   extern SMM_CPU_PRIVATE_DATA  *gSmmCpuPrivate;
>