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

["Laszlo Ersek" <lersek@redhat.com>]

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.

>> @@ -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.

Thanks
Laszlo