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.3732.1575437647624537224 for ; Tue, 03 Dec 2019 21:34:07 -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 orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Dec 2019 21:34:06 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,276,1571727600"; d="scan'208";a="361446098" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga004.jf.intel.com with ESMTP; 03 Dec 2019 21:34:06 -0800 Received: from fmsmsx602.amr.corp.intel.com (10.18.126.82) 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:33:54 -0800 Received: from fmsmsx602.amr.corp.intel.com (10.18.126.82) by fmsmsx602.amr.corp.intel.com (10.18.126.82) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Tue, 3 Dec 2019 21:33:53 -0800 Received: from shsmsx107.ccr.corp.intel.com (10.239.4.96) by fmsmsx602.amr.corp.intel.com (10.18.126.82) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Tue, 3 Dec 2019 21:33:53 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.109]) by SHSMSX107.ccr.corp.intel.com ([169.254.9.164]) with mapi id 14.03.0439.000; Wed, 4 Dec 2019 13:33:52 +0800 From: "Dong, Eric" To: Laszlo Ersek , "devel@edk2.groups.io" CC: "Ni, Ray" , "Gao, Liming" 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: AQHVpfO/hNyhzSbZ/keB9lij2XaGfKehXNHg///hW4CACD5L0A== Date: Wed, 4 Dec 2019 05:33:51 +0000 Message-ID: References: <20191128061716.196-1-eric.dong@intel.com> <7ac1f712-5da3-e6b2-f24e-936feb81daa3@redhat.com> <27cc2a77-2fcf-b242-f588-278a8d1e35ca@redhat.com> In-Reply-To: <27cc2a77-2fcf-b242-f588-278a8d1e35ca@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="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Laszlo, > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Friday, November 29, 2019 3:39 PM > To: Dong, Eric ; devel@edk2.groups.io > Cc: Ni, Ray ; Gao, Liming > Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid > allocate Token every time. >=20 > On 11/29/19 04:02, Dong, Eric wrote: > > Hi Laszlo, > > > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > > Laszlo Ersek > > Sent: Thursday, November 28, 2019 9:57 PM > > To: Dong, Eric ; devel@edk2.groups.io > > Cc: Ni, Ray ; Gao, Liming > > Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: > Avoid allocate Token every time. >=20 > >> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 56 > ++++++++++++++++++++-- > >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 16 +++++++ > >> 2 files changed, 68 insertions(+), 4 deletions(-) > > > > commenting on the header file changes first: > > [Eric] what's this sentence means? Follow above comments to update the > comment message? >=20 > Your patch email included the C source file changes first, and the header= file > changes second. I find that more difficult to reason about than the oppos= ite > order (header first, C source second). >=20 > Therefore, I split your email in two parts, and moved the H file changes = to the > top. And, I commented on those H file changes first. >=20 > >> 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. > >> + // > >> + gSmmCpuPrivate->UsedTokenNum =3D 0; > > > > (6) Here we do not zero out the current token buffer, but in > > CreateToken() and InitializeDataForMmMp(), we use AllocateZeroPool(). > > > > This is an inconsistency, we should call either ZeroMem() here (if > > zeroing matters), or AllocatePool() in the other two places (if > > zeroing does not matter). > > [Eric] Not catch your meaning here? Why can't use "=3D0" here? >=20 > In both CreateToken() and InitializeDataForMmMp(), we perform *three* > actions: > (a) ensure CurrentTokenBuf is allocated, > (b) clear CurrentTokenBuf, > (c) set UsedTokenNum to zero. >=20 > In FreeTokens(), we perform *two* actions: > (a) ensure CurrentTokenBuf is allocated (it needs no explicit action, but= it is > an invariant nonetheless), > (c) set UsedTokenNum to zero. >=20 > Step (b) is missing from FreeTokens(). That's inconsistent with > CreateToken() and InitializeDataForMmMp(). >=20 > The question is whether the following predicate is important or not: >=20 > - "all unused tokens in the current token buffer must be all-bits-zero" >=20 > If this predicate is important, then you should add step (b) to > FreeTokens(): >=20 > ZeroMem ( > gSmmCpuPrivate->CurrentTokenBuf, > SpinLockSize * MAX_PRE_RESERVE_TOKEN_COUNT > ); >=20 > If the predicate is not important, then you should replace the > AllocateZeroPool() calls with AllocatePool(), in CreateToken() and > InitializeDataForMmMp(). >=20 > It is not consistent to clear CurrentTokenBuf in only *some* cases when > UsedTokenNum is set to zero. [[Eric]] In CreateToken function, I add code InitializeSpinLock() to initia= lize the Token space first, so "Clear CurrentTokenBuf" action is not must have i= tem. I will remove the "Zero Memory" action in my next version changes. >=20 > >> @@ -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")); > > > > (7) This is an expected case, and not too much a corner case at that. > > Furthermore, the DEBUG message is in a performance-sensitive path. > > [Eric] this code is called by the caller. I don't think it's > > performance sensitive. What's your rule for "performance-sensitive > > path" ? I add this debug message because I want to know how often the > > pre allocate buffer is not enough. We can enlarge the buffer size to g= et > better performance. >=20 > The patch is about making CreateToken() faster. It's done by allocating > SMRAM less frequently (not on every call to CreateToken()). In some cases > however, CreateToken() will still allocate SMRAM, and that's going to be = a > "spike" in latency, I expect. >=20 > Adding a DEBUG_INFO to that code path makes the spike worse. It does not > affect the throughput of CreateToken() much (the spike is amortized over > MAX_PRE_RESERVE_TOKEN_COUNT calls), but it makes the distribution less > even. I would use DEBUG_VERBOSE to avoid worsening the spike when a > platform build does not ask for DEBUG_VERBOSE explicitly. >=20 > If you disagree, I can live with DEBUG_INFO. >=20 [[Eric]] I add debug message to let us know the frequency of the allocation= action.=20 It make sense to change the level to VERBOSE. I will update it in my next v= ersion changes. Thanks, Eric > Thanks > Laszlo