[edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time.
Dong, Eric
eric.dong at intel.com
Wed Dec 4 05:44:48 UTC 2019
Hi Philippe,
> -----Original Message-----
> From: Philippe Mathieu-Daudé [mailto:philmd at redhat.com]
> Sent: Thursday, November 28, 2019 9:16 PM
> To: devel at edk2.groups.io; Dong, Eric <eric.dong at intel.com>
> Cc: Ni, Ray <ray.ni at intel.com>; Laszlo Ersek <lersek at redhat.com>
> Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid
> allocate Token every time.
>
> 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.
[[Eric]] got it, will use this format in my next version change. Thanks.
>
> Another nitpick, remove the trailing dot in commit subject.
[[Eric]] Update in my next version change.
>
> > 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 at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Laszlo Ersek <lersek at 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."
[[Eric]] Update it in my next version change.
Thanks,
Eric
>
> 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;
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51718): https://edk2.groups.io/g/devel/message/51718
Mute This Topic: https://groups.io/mt/63850169/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list