Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time.

["Dong, Eric" <eric.dong@intel.com>]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, November 29, 2019 3:39 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Liming <liming.gao@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.dong@intel.com>; devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Gao, Liming <liming.gao@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