[edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time.
Dong, Eric via Groups.Io
eric.dong=intel.com at groups.io
Fri Nov 29 03:02:04 UTC 2019
Hi Laszlo,
From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of Laszlo Ersek
Sent: Thursday, November 28, 2019 9:57 PM
To: Dong, Eric <eric.dong at intel.com>; devel at edk2.groups.io
Cc: Ni, Ray <ray.ni at intel.com>; Gao, Liming <liming.gao at intel.com>
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
<https://bugzilla.tianocore.org/show_bug.cgi?id=1937>. 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 <eric.dong at intel.com<mailto:eric.dong at intel.com>>
> Cc: Ray Ni <ray.ni at intel.com<mailto:ray.ni at intel.com>>
> Cc: Laszlo Ersek <lersek at redhat.com<mailto:lersek at redhat.com>>
> ---
> 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);
> }
>
> /**
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51483): https://edk2.groups.io/g/devel/message/51483
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]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20191129/f4a04710/attachment.htm>
More information about the edk2-devel-archive
mailing list