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. On 11/28/19 07:17, Eric Dong wrote: > v2 changes: > Minor update based on comments. > > v1 changes: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2388 > > Current logic allocate Token every time when need to use it. (1) Can you please clarify, in the *commit message*, what service the token allocation is needed for? To me it seems to be related to the SMM MP protocol, which was added for . Is that correct? [Eric] yes, it's a SMI latency issue occurred only in some special case. It's a regression issue caused by SMM MP protocol change. (For example, the CreateToken() function was introduced in commit 51dd408ae102, "UefiCpuPkg/PiSmmCpuDxeSmm: Enable MM MP Protocol", 2019-07-16.) (2) If the problem indeed depends on MM MP protocol usage, then please update Bugzilla 2388 too, with that information ("performance regression from BZ#1937"). If a platform does not use the MM MP protocol, then highlighting this information helps platforms determine that they are not affected. (I read Bugzilla 2388 soon after it was filed, and I was surprised, because I didn't remember any "human visible" SMI handling slowdown. Maybe it's not on the "human visible" scale; I'm not sure.) [Eric] Yes, it's a special case. I check the basic SMI (write data to B2 and collect the performance data) latency data and not found this issue. I found you already add note in this bugz. Thanks. > 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(-) commenting on the header file changes first: [Eric] what's this sentence means? Follow above comments to update the comment message? > 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 (3) This is a very strange constant, 0x512: it's decimal 1298. Is that your intent? [Eric] add "0x" by mistake, will use PCD and remove this code. (4) Should we make this a global (SMRAM) variable instead, and initialize it from a PCD? [Eric] yes, will use PCD for this value. I'm not sure how large tokens are, but if a platform has no use for the [Eric] The value is 0x40 for the platform I used. MM MP protocol, then pre-allocating tokens for the MM MP protocol just wastes SMRAM. If I grep edk2 (at bd85bf54c268), and edk2-platforms (at 04889ec1198b), for "gEfiMmMpProtocolGuid", the only reported module is "UefiCpuPkg/PiSmmCpuDxeSmm" itself. In other words, there is no open source consumer for the new protocol. I don't think we should waste SMRAM on platforms that don't consume this protocol. Furthermore, I've applied the following patch, and rebuilt OVMF: > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index 0685637c2b51..5874195ba96e 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -1119,6 +1119,8 @@ CreateToken ( > SPIN_LOCK *CpuToken; > UINTN SpinLockSize; > > + ASSERT (FALSE); > + > SpinLockSize = GetSpinLockProperties (); > CpuToken = AllocatePool (SpinLockSize); > ASSERT (CpuToken != NULL); then I've re-run my usual tests. The ASSERT() has not been reached, and so I think that the new SMRAM allocation would be pure loss (= unused) for OVMF. I suggest that we introduce a new PCD for "token count per chunk", and set the default value to 1, in the DEC file. (And we should copy the PCD into a global variable, in InitializeDataForMmMp().) This way, if a platform suddenly starts consuming tokens, it will work (there will always be a single available token). If they want to improve performance (using a chunked allocation style), they can set the PCD as they see fit. Back to the patch: On 11/28/19 07:17, Eric Dong wrote: > // > // 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; > (5) There are two new lines above that are overlong. Did "PatchCheck.py" not complain? [Eric] I think PatchCheck tool not check the code width, only check the commit message width. Also edk2 coding style spec not has restrict rule for it, just a general rule. Content like below: 5.1 General Rules 5.1.1 Lines shall be 120 columns, or less https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications > > 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 = 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 "=0" here? > + > + 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")); (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 get better performance. Therefore, the message should be logged with the DEBUG_VERBOSE mask, and not with the DEBUG_INFO mask. (8) In addition, the DEBUG line is too long. [Eric] Same with [5], I think not break edk2 rules. (9) The DEBUG line also misses a space character. [Eric] will update in next version. > + > + // > + // 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); (10) Two lines are overlong. [Eric] Same with [5], I think not break edk2 rules. > + 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)); (11) For consistency with (7), you might want to downgrade this to DEBUG_VERBOSE too. But, in this case, I won't insist. [Eric] This value impact the final used buffer size, want to know this info in normal boot. So I use INFO level. (12) Space character missing after "DEBUG". [Eric] Will update it in next patch. (13) Are you proposing this patch for edk2-stable201911? (CC Liming) [Eric] it's too later, will not propose in this stable tag. Thanks Laszlo > + > + 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); > } > > /**