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

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

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