From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mx.groups.io with SMTP id smtpd.web09.6.1575438291324573860 for ; Tue, 03 Dec 2019 21:44:51 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.88, mailfrom: eric.dong@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Dec 2019 21:44:51 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,276,1571727600"; d="scan'208";a="208737167" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga007.fm.intel.com with ESMTP; 03 Dec 2019 21:44:51 -0800 Received: from shsmsx105.ccr.corp.intel.com (10.239.4.158) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 3 Dec 2019 21:44:50 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.109]) by SHSMSX105.ccr.corp.intel.com ([169.254.11.72]) with mapi id 14.03.0439.000; Wed, 4 Dec 2019 13:44:48 +0800 From: "Dong, Eric" To: =?iso-8859-1?Q?Philippe_Mathieu-Daud=E9?= , "devel@edk2.groups.io" CC: "Ni, Ray" , Laszlo Ersek Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time. Thread-Topic: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time. Thread-Index: AQHVpe3qhNyhzSbZ/keB9lij2XaGfKepf6Sw Date: Wed, 4 Dec 2019 05:44:48 +0000 Message-ID: References: <20191128061716.196-1-eric.dong@intel.com> <515bc9a6-106f-4e8d-c5da-e8197990fe13@redhat.com> In-Reply-To: <515bc9a6-106f-4e8d-c5da-e8197990fe13@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: eric.dong@intel.com Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Hi Philippe, > -----Original Message----- > From: Philippe Mathieu-Daud=E9 [mailto:philmd@redhat.com] > Sent: Thursday, November 28, 2019 9:16 PM > To: devel@edk2.groups.io; Dong, Eric > Cc: Ni, Ray ; Laszlo Ersek > Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid > allocate Token every time. >=20 > Hi Eric, >=20 > On 11/28/19 7:17 AM, Dong, Eric via Groups.Io wrote: > > v2 changes: > > Minor update based on comments. > > > > v1 changes: >=20 > 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. >=20 > Another nitpick, remove the trailing dot in commit subject. [[Eric]] Update in my next version change. >=20 > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2388 > > > > 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 > > Cc: Ray Ni > > Cc: Laszlo Ersek > > --- > > 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. >=20 > 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 >=20 > Patch looks good otherwise. >=20 > > + // > > + gSmmCpuPrivate->UsedTokenNum =3D 0; > > + > > + Link =3D GetFirstNode (&gSmmCpuPrivate->OldTokenBufList); > > + while (!IsNull (&gSmmCpuPrivate->OldTokenBufList, Link)) { > > + TokenBuf =3D TOKEN_BUFFER_FROM_LINK (Link); > > + > > + Link =3D RemoveEntryList (&TokenBuf->Link); > > + > > + FreePool (TokenBuf->Buffer); > > + FreePool (TokenBuf); > > + } > > > > while (!IsListEmpty (&gSmmCpuPrivate->TokenList)) { > > Link =3D 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 =3D GetSpinLockProperties (); > > - CpuToken =3D AllocatePool (SpinLockSize); > > - ASSERT (CpuToken !=3D NULL); > > + > > + if (gSmmCpuPrivate->UsedTokenNum =3D=3D > 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 =3D AllocatePool (sizeof (TOKEN_BUFFER)); > > + ASSERT (TokenBuf !=3D NULL); > > + TokenBuf->Signature =3D TOKEN_BUFFER_SIGNATURE; > > + TokenBuf->Buffer =3D gSmmCpuPrivate->CurrentTokenBuf; > > + > > + InsertTailList (&gSmmCpuPrivate->OldTokenBufList, > > + &TokenBuf->Link); > > + > > + gSmmCpuPrivate->CurrentTokenBuf =3D AllocateZeroPool (SpinLockSi= ze > * MAX_PRE_RESERVE_TOKEN_COUNT); > > + ASSERT (gSmmCpuPrivate->CurrentTokenBuf !=3D NULL); > > + gSmmCpuPrivate->UsedTokenNum =3D 0; } > > + > > + CpuToken =3D (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + > > + SpinLockSize * gSmmCpuPrivate->UsedTokenNum); > > + gSmmCpuPrivate->UsedTokenNum++; > > + > > InitializeSpinLock (CpuToken); > > AcquireSpinLock (CpuToken); > > > > @@ -1737,10 +1775,20 @@ InitializeDataForMmMp ( > > VOID > > ) > > { > > + UINTN SpinLockSize; > > + > > + SpinLockSize =3D GetSpinLockProperties (); DEBUG((DEBUG_INFO, > > + "CpuSmm: SpinLock Size =3D 0x%x\n", SpinLockSize)); > > + > > + gSmmCpuPrivate->CurrentTokenBuf =3D AllocateZeroPool (SpinLockSize * > > + MAX_PRE_RESERVE_TOKEN_COUNT); ASSERT > > + (gSmmCpuPrivate->CurrentTokenBuf !=3D NULL); > > + gSmmCpuPrivate->UsedTokenNum =3D 0; > > + > > gSmmCpuPrivate->ApWrapperFunc =3D AllocatePool (sizeof > (PROCEDURE_WRAPPER) * gSmmCpuPrivate- > >SmmCoreEntryContext.NumberOfCpus); > > ASSERT (gSmmCpuPrivate->ApWrapperFunc !=3D 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 tok= ens used in > CurrentTokenBuf. > > } SMM_CPU_PRIVATE_DATA; > > > > extern SMM_CPU_PRIVATE_DATA *gSmmCpuPrivate; > >