From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mx.groups.io with SMTP id smtpd.web09.2817.1574916192692884850 for ; Wed, 27 Nov 2019 20:43:12 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.24, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Nov 2019 20:43:12 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,252,1571727600"; d="scan'208";a="217520153" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga001.fm.intel.com with ESMTP; 27 Nov 2019 20:43:11 -0800 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 27 Nov 2019 20:43:11 -0800 Received: from shsmsx105.ccr.corp.intel.com (10.239.4.158) by fmsmsx109.amr.corp.intel.com (10.18.116.9) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 27 Nov 2019 20:43:08 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.127]) by SHSMSX105.ccr.corp.intel.com ([169.254.11.225]) with mapi id 14.03.0439.000; Thu, 28 Nov 2019 12:43:07 +0800 From: "Ni, Ray" To: "devel@edk2.groups.io" , "Dong, Eric" CC: Laszlo Ersek Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time. Thread-Topic: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time. Thread-Index: AQHVpZe1T68DdmqA+Uq8jSBaliP04aef/nJQ Date: Thu, 28 Nov 2019 04:43:07 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C371407@SHSMSX104.ccr.corp.intel.com> References: <20191128025814.417-1-eric.dong@intel.com> In-Reply-To: <20191128025814.417-1-eric.dong@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: 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 > Subject: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid > allocate Token every time. >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2388 >=20 > 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. >=20 > Signed-off-by: Eric Dong >=20 > Cc: Eric Dong >=20 > Cc: Laszlo Ersek > --- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 56 > +++++++++++++++++++--- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 16 +++++++ > 2 files changed, 66 insertions(+), 6 deletions(-) >=20 > 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 ( > { >=20 > LIST_ENTRY *Link; >=20 > PROCEDURE_TOKEN *ProcToken; >=20 > + TOKEN_BUFFER *TokenBuf; >=20 > + >=20 > + // >=20 > + // Only reset current used Token buffer, not free this buffer. >=20 > + // >=20 > + gSmmCpuPrivate->UsedTokenNum =3D 0; >=20 > + >=20 > + Link =3D GetFirstNode (&gSmmCpuPrivate->OldTokenBufList); >=20 > + while (!IsNull (&gSmmCpuPrivate->OldTokenBufList, Link)) { >=20 > + TokenBuf =3D TOKEN_BUFFER_FROM_LINK (Link); >=20 > + >=20 > + Link =3D GetNextNode (&gSmmCpuPrivate->OldTokenBufList, Link); >=20 > + >=20 > + RemoveEntryList (&TokenBuf->Link); 1. The above two lines can be combined to below single line: Link =3D RemoveEntryList (&TokenBuf->Link); >=20 > + >=20 > + FreePool (TokenBuf->Buffer); >=20 > + FreePool (TokenBuf); >=20 > + } >=20 >=20 >=20 > while (!IsListEmpty (&gSmmCpuPrivate->TokenList)) { >=20 > Link =3D GetFirstNode (&gSmmCpuPrivate->TokenList); >=20 > @@ -499,7 +517,6 @@ FreeTokens ( >=20 >=20 > RemoveEntryList (&ProcToken->Link); >=20 >=20 >=20 > - FreePool ((VOID *)ProcToken->ProcedureToken); >=20 > FreePool (ProcToken); >=20 > } >=20 > } >=20 > @@ -1115,13 +1132,32 @@ CreateToken ( > VOID >=20 > ) >=20 > { >=20 > - PROCEDURE_TOKEN *ProcToken; >=20 > + PROCEDURE_TOKEN *ProcToken; >=20 > SPIN_LOCK *CpuToken; >=20 > - UINTN SpinLockSize; >=20 > + TOKEN_BUFFER *TokenBuf; >=20 > + >=20 > + if (gSmmCpuPrivate->UsedTokenNum >=3D MAX_TOKEN_COUND_NUMBER > - 1) { 2. Can the macro be MAX_TOKEN_COUNT? The variable be " gSmmCpuPrivate->Toke= nCount"? Usually when we define a storage, there are three variables: Elements[], Co= unt, Capacity. MAX_TOKEN_COUNT is the capacity. TokenCount is the count. 3. should be "TokenCount =3D=3D MAX_TOKEN_COUNT". 4. Can you explain how the toke buffer is managed using comments? >=20 > + DEBUG((DEBUG_INFO, "Token buffer not enough, allocate new > buffer[TokenNum=3D0x%x]!\n", gSmmCpuPrivate->UsedTokenNum)); 5. Suggest add prefix like "CpuSmm: ". >=20 > + >=20 > + // >=20 > + // Save Current Token Buffer to the list. >=20 > + // >=20 > + TokenBuf =3D AllocatePool (sizeof (TOKEN_BUFFER)); >=20 > + ASSERT (TokenBuf !=3D NULL); >=20 > + TokenBuf->Signature =3D TOKEN_BUFFER_SIGNATURE; >=20 > + TokenBuf->Buffer =3D gSmmCpuPrivate->CurrentTokenBuf; >=20 > + >=20 > + InsertTailList (&gSmmCpuPrivate->OldTokenBufList, &TokenBuf->Link); >=20 > + >=20 > + gSmmCpuPrivate->CurrentTokenBuf =3D AllocateZeroPool > (gSmmCpuPrivate->TokenSize * MAX_TOKEN_COUND_NUMBER); >=20 > + ASSERT (gSmmCpuPrivate->CurrentTokenBuf !=3D NULL); >=20 > + gSmmCpuPrivate->UsedTokenNum =3D 0; >=20 > + } >=20 > + >=20 > + CpuToken =3D (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + > gSmmCpuPrivate->TokenSize * gSmmCpuPrivate->UsedTokenNum); >=20 > + gSmmCpuPrivate->UsedTokenNum++; >=20 > + //DEBUG((DEBUG_INFO, "Token Address =3D 0x%x, Token Number =3D > 0x%x\n", CpuToken, gSmmCpuPrivate->UsedTokenNum)); >=20 >=20 >=20 > - SpinLockSize =3D GetSpinLockProperties (); >=20 > - CpuToken =3D AllocatePool (SpinLockSize); >=20 > - ASSERT (CpuToken !=3D NULL); >=20 > InitializeSpinLock (CpuToken); >=20 > AcquireSpinLock (CpuToken); >=20 >=20 >=20 > @@ -1737,10 +1773,18 @@ InitializeDataForMmMp ( > VOID >=20 > ) >=20 > { >=20 > + gSmmCpuPrivate->TokenSize =3D (UINT32)GetSpinLockProperties (); 6. How about not to cache the TokenSize? >=20 > + DEBUG((DEBUG_INFO, "gSmmCpuPrivate->TokenSize =3D 0x%x\n", > gSmmCpuPrivate->TokenSize)); 7. Maybe no need. >=20 > + >=20 > + gSmmCpuPrivate->CurrentTokenBuf =3D AllocateZeroPool > (gSmmCpuPrivate->TokenSize * MAX_TOKEN_COUND_NUMBER); >=20 > + ASSERT (gSmmCpuPrivate->CurrentTokenBuf !=3D NULL); >=20 > + gSmmCpuPrivate->UsedTokenNum =3D 0; >=20 > + >=20 > gSmmCpuPrivate->ApWrapperFunc =3D AllocatePool (sizeof > (PROCEDURE_WRAPPER) * gSmmCpuPrivate- > >SmmCoreEntryContext.NumberOfCpus); >=20 > ASSERT (gSmmCpuPrivate->ApWrapperFunc !=3D NULL); >=20 >=20 >=20 > InitializeListHead (&gSmmCpuPrivate->TokenList); >=20 > + InitializeListHead (&gSmmCpuPrivate->OldTokenBufList); >=20 > } >=20 >=20 >=20 > /** >=20 > 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 >=20 > #define ARRIVAL_EXCEPTION_SMI_DISABLED 0x4 >=20 >=20 >=20 > +#define MAX_TOKEN_COUND_NUMBER 0x512 >=20 > // >=20 > // Wrapper used to convert EFI_AP_PROCEDURE2 and EFI_AP_PROCEDURE. >=20 > // >=20 > @@ -217,6 +218,17 @@ typedef struct { >=20 >=20 > #define PROCEDURE_TOKEN_FROM_LINK(a) CR (a, PROCEDURE_TOKEN, > Link, PROCEDURE_TOKEN_SIGNATURE) >=20 >=20 >=20 > +#define TOKEN_BUFFER_SIGNATURE SIGNATURE_32 ('T', 'K', 'B', 'S') >=20 > + >=20 > +typedef struct { >=20 > + UINTN Signature; >=20 > + LIST_ENTRY Link; >=20 > + >=20 > + UINT8 *Buffer; >=20 > +} TOKEN_BUFFER; >=20 > + >=20 > +#define TOKEN_BUFFER_FROM_LINK(a) CR (a, TOKEN_BUFFER, Link, > TOKEN_BUFFER_SIGNATURE) >=20 > + >=20 > // >=20 > // Private structure for the SMM CPU module that is stored in DXE Runtim= e > memory >=20 > // Contains the SMM Configuration Protocols that is produced. >=20 > @@ -243,6 +255,10 @@ typedef struct { > PROCEDURE_WRAPPER *ApWrapperFunc; >=20 > LIST_ENTRY TokenList; >=20 >=20 >=20 > + LIST_ENTRY OldTokenBufList; >=20 > + UINT8 *CurrentTokenBuf; >=20 > + UINTN UsedTokenNum; // Only record token= s used in > CurrentTokenBuf. >=20 > + UINT32 TokenSize; >=20 > } SMM_CPU_PRIVATE_DATA; >=20 >=20 >=20 > extern SMM_CPU_PRIVATE_DATA *gSmmCpuPrivate; >=20 > -- > 2.23.0.windows.1 >=20 >=20 > -=3D-=3D-=3D-=3D-=3D-=3D > Groups.io Links: You receive all messages sent to this group. >=20 > 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] > -=3D-=3D-=3D-=3D-=3D-=3D