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

["Laszlo Ersek" <lersek@redhat.com>]

On 01/02/20 04:30, Ni, Ray wrote:
> Reviewed-by: Ray Ni <ray.ni@intel.com>

Eric, please go ahead with Ray's R-b.

Thanks
Laszlo

> 
>> -----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
>