Re: [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Pre-allocate PROCEDURE_TOKEN buffer

["Ni, Ray" <ray.ni@intel.com>]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Dong, Eric <eric.dong@intel.com>
> Sent: Friday, December 27, 2019 3:49 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Pre-allocate
> PROCEDURE_TOKEN buffer
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2388
> 
> Token is new introduced by MM MP Protocol. 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.
> 
> Former change (9caaa79dd7e078ebb4012dde3b3d3a5d451df609) missed
> PROCEDURE_TOKEN part, this change covers it.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> 
> v2 changes:
> 
>   Remove the not used variable.
> 
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      | 190 ++++++++++++-------
> --
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |   6 +-
>  2 files changed, 109 insertions(+), 87 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 4808045f71..870250b0c5 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -429,38 +429,29 @@ ReleaseToken (
> 
> 
>  **/
> 
>  VOID
> 
> -FreeTokens (
> 
> +ResetTokens (
> 
>    VOID
> 
>    )
> 
>  {
> 
>    LIST_ENTRY            *Link;
> 
>    PROCEDURE_TOKEN       *ProcToken;
> 
> -  TOKEN_BUFFER          *TokenBuf;
> 
> 
> 
> -  //
> 
> -  // Only free the token buffer recorded in the OldTOkenBufList
> 
> -  // upon exiting SMI. Current token buffer stays allocated so
> 
> -  // next SMI doesn't need to re-allocate.
> 
> -  //
> 
> -  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);
> 
> +  Link = GetFirstNode (&gSmmCpuPrivate->TokenList);
> 
> +  while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) {
> 
>      ProcToken = PROCEDURE_TOKEN_FROM_LINK (Link);
> 
> 
> 
> -    RemoveEntryList (&ProcToken->Link);
> 
> +    ProcToken->RunningApCount = 0;
> 
> +    ProcToken->Used = FALSE;
> 
> +
> 
> +    //
> 
> +    // Check the spinlock status and release it if not released yet.
> 
> +    //
> 
> +    if (!AcquireSpinLockOrFail(ProcToken->SpinLock)) {
> 
> +      DEBUG((DEBUG_ERROR, "Risk::SpinLock still not released!"));
> 
> +    }
> 
> +    ReleaseSpinLock (ProcToken->SpinLock);
> 
> 
> 
> -    FreePool (ProcToken);
> 
> +    Link = GetNextNode (&gSmmCpuPrivate->TokenList, Link);
> 
>    }
> 
>  }
> 
> 
> 
> @@ -685,9 +676,9 @@ BSPHandler (
>    WaitForAllAPs (ApCount);
> 
> 
> 
>    //
> 
> -  // Clean the tokens buffer.
> 
> +  // Reset the tokens buffer.
> 
>    //
> 
> -  FreeTokens ();
> 
> +  ResetTokens ();
> 
> 
> 
>    //
> 
>    // Reset BspIndex to -1, meaning BSP has not been elected.
> 
> @@ -1056,7 +1047,7 @@ IsTokenInUse (
>    while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) {
> 
>      ProcToken = PROCEDURE_TOKEN_FROM_LINK (Link);
> 
> 
> 
> -    if (ProcToken->SpinLock == Token) {
> 
> +    if (ProcToken->Used && ProcToken->SpinLock == Token) {
> 
>        return TRUE;
> 
>      }
> 
> 
> 
> @@ -1067,61 +1058,112 @@ IsTokenInUse (
>  }
> 
> 
> 
>  /**
> 
> -  create token and save it to the maintain list.
> 
> -
> 
> -  @param     RunningApCount   Input running AP count.
> 
> -
> 
> -  @retval    return the spin lock used as token.
> 
> +  Allocate buffer for the SPIN_LOCK and PROCEDURE_TOKEN.
> 
> 
> 
>  **/
> 
> -PROCEDURE_TOKEN *
> 
> -CreateToken (
> 
> -  IN UINT32              RunningApCount
> 
> +VOID
> 
> +AllocateTokenBuffer (
> 
> +  VOID
> 
>    )
> 
>  {
> 
> -  PROCEDURE_TOKEN     *ProcToken;
> 
> -  SPIN_LOCK           *SpinLock;
> 
>    UINTN               SpinLockSize;
> 
> -  TOKEN_BUFFER        *TokenBuf;
> 
>    UINT32              TokenCountPerChunk;
> 
> +  UINTN               ProcTokenSize;
> 
> +  UINTN               Index;
> 
> +  PROCEDURE_TOKEN     *ProcToken;
> 
> +  SPIN_LOCK           *SpinLock;
> 
> +  UINT8               *SpinLockBuffer;
> 
> +  UINT8               *ProcTokenBuffer;
> 
> 
> 
>    SpinLockSize = GetSpinLockProperties ();
> 
> +  ProcTokenSize = sizeof (PROCEDURE_TOKEN);
> 
> +
> 
>    TokenCountPerChunk = FixedPcdGet32
> (PcdCpuSmmMpTokenCountPerChunk);
> 
> +  ASSERT (TokenCountPerChunk != 0);
> 
> +  if (TokenCountPerChunk == 0) {
> 
> +    DEBUG ((DEBUG_ERROR, "PcdCpuSmmMpTokenCountPerChunk should
> not be Zero!\n"));
> 
> +    CpuDeadLoop ();
> 
> +  }
> 
> +  DEBUG ((DEBUG_INFO, "CpuSmm: SpinLock Size = 0x%x,
> PcdCpuSmmMpTokenCountPerChunk = 0x%x\n", SpinLockSize,
> TokenCountPerChunk));
> 
> 
> 
> -  if (gSmmCpuPrivate->UsedTokenNum == TokenCountPerChunk) {
> 
> -    DEBUG ((DEBUG_VERBOSE, "CpuSmm: No free token buffer, allocate new
> buffer!\n"));
> 
> +  //
> 
> +  // Separate the Spin_lock and Proc_token because the alignment requires
> by Spin_Lock.
> 
> +  //
> 
> +  SpinLockBuffer = AllocatePool (SpinLockSize * TokenCountPerChunk);
> 
> +  ASSERT (SpinLockBuffer != NULL);
> 
> 
> 
> -    //
> 
> -    // 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;
> 
> +  ProcTokenBuffer = AllocatePool (ProcTokenSize * TokenCountPerChunk);
> 
> +  ASSERT (ProcTokenBuffer != NULL);
> 
> +
> 
> +  for (Index = 0; Index < TokenCountPerChunk; Index++) {
> 
> +    SpinLock = (SPIN_LOCK *)(SpinLockBuffer + SpinLockSize * Index);
> 
> +    InitializeSpinLock (SpinLock);
> 
> +
> 
> +    ProcToken = (PROCEDURE_TOKEN *)(ProcTokenBuffer + ProcTokenSize *
> Index);
> 
> +    ProcToken->Signature = PROCEDURE_TOKEN_SIGNATURE;
> 
> +    ProcToken->SpinLock = SpinLock;
> 
> +    ProcToken->Used = FALSE;
> 
> +    ProcToken->RunningApCount = 0;
> 
> +
> 
> +    InsertTailList (&gSmmCpuPrivate->TokenList, &ProcToken->Link);
> 
> +  }
> 
> +}
> 
> 
> 
> -    InsertTailList (&gSmmCpuPrivate->OldTokenBufList, &TokenBuf->Link);
> 
> +/**
> 
> +  Find first free token in the allocated token list.
> 
> +
> 
> +  @retval    return the first free PROCEDURE_TOKEN.
> 
> +
> 
> +**/
> 
> +PROCEDURE_TOKEN *
> 
> +FindFirstFreeToken (
> 
> +  VOID
> 
> +  )
> 
> +{
> 
> +  LIST_ENTRY        *Link;
> 
> +  PROCEDURE_TOKEN   *ProcToken;
> 
> 
> 
> -    gSmmCpuPrivate->CurrentTokenBuf = AllocatePool (SpinLockSize *
> TokenCountPerChunk);
> 
> -    ASSERT (gSmmCpuPrivate->CurrentTokenBuf != NULL);
> 
> -    gSmmCpuPrivate->UsedTokenNum = 0;
> 
> +  Link = GetFirstNode (&gSmmCpuPrivate->TokenList);
> 
> +  while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) {
> 
> +    ProcToken = PROCEDURE_TOKEN_FROM_LINK (Link);
> 
> +
> 
> +    if (!ProcToken->Used) {
> 
> +      return ProcToken;
> 
> +    }
> 
> +
> 
> +    Link = GetNextNode (&gSmmCpuPrivate->TokenList, Link);
> 
>    }
> 
> 
> 
> -  SpinLock = (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf +
> SpinLockSize * gSmmCpuPrivate->UsedTokenNum);
> 
> -  gSmmCpuPrivate->UsedTokenNum++;
> 
> +  return NULL;
> 
> +}
> 
> +
> 
> +/**
> 
> +  Get the free token.
> 
> +
> 
> +  If no free token, allocate new tokens then return the free one.
> 
> +
> 
> +  @retval    return the first free PROCEDURE_TOKEN.
> 
> 
> 
> -  InitializeSpinLock (SpinLock);
> 
> -  AcquireSpinLock (SpinLock);
> 
> +**/
> 
> +PROCEDURE_TOKEN *
> 
> +GetFreeToken (
> 
> +  IN UINT32       RunningApsCount
> 
> +  )
> 
> +{
> 
> +  PROCEDURE_TOKEN  *NewToken;
> 
> 
> 
> -  ProcToken = AllocatePool (sizeof (PROCEDURE_TOKEN));
> 
> -  ASSERT (ProcToken != NULL);
> 
> -  ProcToken->Signature = PROCEDURE_TOKEN_SIGNATURE;
> 
> -  ProcToken->SpinLock = SpinLock;
> 
> -  ProcToken->RunningApCount = RunningApCount;
> 
> +  NewToken = FindFirstFreeToken ();
> 
> +  if (NewToken == NULL) {
> 
> +    AllocateTokenBuffer ();
> 
> +    NewToken = FindFirstFreeToken ();
> 
> +  }
> 
> +  ASSERT (NewToken != NULL);
> 
> 
> 
> -  InsertTailList (&gSmmCpuPrivate->TokenList, &ProcToken->Link);
> 
> +  NewToken->Used = TRUE;
> 
> +  NewToken->RunningApCount = RunningApsCount;
> 
> +  AcquireSpinLock (NewToken->SpinLock);
> 
> 
> 
> -  return ProcToken;
> 
> +  return NewToken;
> 
>  }
> 
> 
> 
>  /**
> 
> @@ -1231,7 +1273,7 @@ InternalSmmStartupThisAp (
>    mSmmMpSyncData->CpuData[CpuIndex].Procedure = Procedure;
> 
>    mSmmMpSyncData->CpuData[CpuIndex].Parameter = ProcArguments;
> 
>    if (Token != NULL) {
> 
> -    ProcToken= CreateToken (1);
> 
> +    ProcToken= GetFreeToken (1);
> 
>      mSmmMpSyncData->CpuData[CpuIndex].Token = ProcToken;
> 
>      *Token = (MM_COMPLETION)ProcToken->SpinLock;
> 
>    }
> 
> @@ -1320,7 +1362,7 @@ InternalSmmStartupAllAPs (
>    }
> 
> 
> 
>    if (Token != NULL) {
> 
> -    ProcToken = CreateToken ((UINT32)mMaxNumberOfCpus);
> 
> +    ProcToken = GetFreeToken ((UINT32)mMaxNumberOfCpus);
> 
>      *Token = (MM_COMPLETION)ProcToken->SpinLock;
> 
>    } else {
> 
>      ProcToken = NULL;
> 
> @@ -1732,28 +1774,12 @@ InitializeDataForMmMp (
>    VOID
> 
>    )
> 
>  {
> 
> -  UINTN              SpinLockSize;
> 
> -  UINT32             TokenCountPerChunk;
> 
> -
> 
> -  SpinLockSize = GetSpinLockProperties ();
> 
> -  TokenCountPerChunk = FixedPcdGet32
> (PcdCpuSmmMpTokenCountPerChunk);
> 
> -  ASSERT (TokenCountPerChunk != 0);
> 
> -  if (TokenCountPerChunk == 0) {
> 
> -    DEBUG ((DEBUG_ERROR, "PcdCpuSmmMpTokenCountPerChunk should
> not be Zero!\n"));
> 
> -    CpuDeadLoop ();
> 
> -  }
> 
> -  DEBUG ((DEBUG_INFO, "CpuSmm: SpinLock Size = 0x%x,
> PcdCpuSmmMpTokenCountPerChunk = 0x%x\n", SpinLockSize,
> TokenCountPerChunk));
> 
> -
> 
> -  gSmmCpuPrivate->CurrentTokenBuf = AllocatePool (SpinLockSize *
> TokenCountPerChunk);
> 
> -  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);
> 
> +
> 
> +  AllocateTokenBuffer ();
> 
>  }
> 
> 
> 
>  /**
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 5c98494e2c..33b3dd140e 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -214,6 +214,7 @@ typedef struct {
> 
> 
>    SPIN_LOCK               *SpinLock;
> 
>    volatile UINT32         RunningApCount;
> 
> +  BOOLEAN                 Used;
> 
>  } PROCEDURE_TOKEN;
> 
> 
> 
>  #define PROCEDURE_TOKEN_FROM_LINK(a)  CR (a, PROCEDURE_TOKEN,
> Link, PROCEDURE_TOKEN_SIGNATURE)
> 
> @@ -254,11 +255,6 @@ typedef struct {
> 
> 
>    PROCEDURE_WRAPPER               *ApWrapperFunc;
> 
>    LIST_ENTRY                      TokenList;
> 
> -
> 
> -  LIST_ENTRY                      OldTokenBufList;
> 
> -
> 
> -  UINT8                           *CurrentTokenBuf;
> 
> -  UINT32                          UsedTokenNum;     // Only record tokens used in
> CurrentTokenBuf.
> 
>  } SMM_CPU_PRIVATE_DATA;
> 
> 
> 
>  extern SMM_CPU_PRIVATE_DATA  *gSmmCpuPrivate;
> 
> --
> 2.23.0.windows.1