Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Friday, November 29, 2019 3:39 PM
> To: Dong, Eric <eric.d...@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray...@intel.com>; Gao, Liming <liming....@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid
> allocate Token every time.
>
> 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 <eric.d...@intel.com>; devel@edk2.groups.io
> > Cc: Ni, Ray <ray...@intel.com>; Gao, Liming <liming....@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm:
> Avoid allocate Token every time.
>
> >> 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?
>
> 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 opposite
> order (header first, C source second).
>
> 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.
>
> >> 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?
>
> In both CreateToken() and InitializeDataForMmMp(), we perform *three*
> actions:
> (a) ensure CurrentTokenBuf is allocated,
> (b) clear CurrentTokenBuf,
> (c) set UsedTokenNum to zero.
>
> 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.
>
> Step (b) is missing from FreeTokens(). That's inconsistent with
> CreateToken() and InitializeDataForMmMp().
>
> The question is whether the following predicate is important or not:
>
> - "all unused tokens in the current token buffer must be all-bits-zero"
>
> If this predicate is important, then you should add step (b) to
> FreeTokens():
>
> ZeroMem (
> gSmmCpuPrivate->CurrentTokenBuf,
> SpinLockSize * MAX_PRE_RESERVE_TOKEN_COUNT
> );
>
> If the predicate is not important, then you should replace the
> AllocateZeroPool() calls with AllocatePool(), in CreateToken() and
> InitializeDataForMmMp().
>
> 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 initialize
the Token space first, so "Clear CurrentTokenBuf" action is not must have item.
I will remove the "Zero Memory" action in my next version changes.
>
> >> @@ -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.
>
> 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.
>
> 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.
>
> If you disagree, I can live with DEBUG_INFO.
>
[[Eric]] I add debug message to let us know the frequency of the allocation
action.
It make sense to change the level to VERBOSE. I will update it in my next
version changes.
Thanks,
Eric
> Thanks
> Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51717): https://edk2.groups.io/g/devel/message/51717
Mute This Topic: https://groups.io/mt/63850169/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
