Re: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Laszlo Ersek <lersek@redhat.com>]

On 07/05/18 15:04, Laszlo Ersek wrote:
> Hi Jeff,
> 
> On 07/04/18 11:39, Fan Jeff wrote:
>> Eric,
>>
>> Current implementation does not call GetApicid() many times,  Please correct you commit message. Your fix is to improve the performance against the current implementation.
> 
> I think the original commit message does make sense. Without the patch,
> GetProcessorNumber() may call GetApicId() up to TotalProcessorNumber
> times. With the patch, even if we skip the stack range search,
> GetProcessorNumber() will call GetApicId() just once.
> 
> [...]
> 
> Some more questions below, for the patch:
> 
>> 发件人: Eric Dong <eric.dong@intel.com>
>> 发送时间: Wednesday, July 4, 2018 4:37:36 PM
>> 收件人: edk2-devel@lists.01.org
>> 抄送: Ruiyu Ni; Jeff Fan; Laszlo Ersek
>> 主题: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.
>>
>> Current function has low performance because it calls GetApicId
>> many times.
>>
>> New logic first try to base on the stack range used by AP to
>> find the processor number. If this solution failed, then call
>> GetApicId once and base on this value to search the processor.
>>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Jeff Fan <vanjeff_919@hotmail.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>> ---
>>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 25 ++++++++++++++++++++++---
>>  1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>> index eb2765910c..abd65bee1a 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>> @@ -418,7 +418,8 @@ ApInitializeSync (
>>  }
>>
>>  /**
>> -  Find the current Processor number by APIC ID.
>> +  First try to find the current Processor number by stack address,
>> +  if it failed, then base on APIC ID.
>>
>>    @param[in]  CpuMpData         Pointer to PEI CPU MP Data
>>    @param[out] ProcessorNumber   Return the pocessor number found
>> @@ -435,16 +436,34 @@ GetProcessorNumber (
>>    UINTN                   TotalProcessorNumber;
>>    UINTN                   Index;
>>    CPU_INFO_IN_HOB         *CpuInfoInHob;
>> +  UINT32                  CurrentApicId;
>>
>> +  TotalProcessorNumber = CpuMpData->CpuCount;
>>    CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
>>
>> -  TotalProcessorNumber = CpuMpData->CpuCount;
>> +  //
>> +  // First try to base on current stack address to find the AP index.
>> +  // &TotalProcessorNumber value located in the stack range.
>> +  //
>>    for (Index = 0; Index < TotalProcessorNumber; Index ++) {
>> -    if (CpuInfoInHob[Index].ApicId == GetApicId ()) {
>> +    if ((CpuInfoInHob[Index].ApTopOfStack > (UINTN) (&TotalProcessorNumber)) &&
>> +        (CpuInfoInHob[Index].ApTopOfStack - CpuMpData->CpuApStackSize < (UINTN) (&TotalProcessorNumber))) {
>>        *ProcessorNumber = Index;
>>        return EFI_SUCCESS;
>>      }
>>    }
> 
> (1) If I understand correctly, ApTopOfStack is the exclusive end
> (highest address) of the AP stack, so any local variable is supposed to
> start strictly below it (the stack grows down). This seems to justify
> the ">" relational operator, in the first subcondition; OK.
> 
> However, what guarantees that the TotalProcessorNumber local variable is
> not located exactly at the (inclusive) base of the AP stack? IOW, why is
> "<" correct, in the second subcondition, rather than "<="?
> 
> 
> (2) I'm generally unhappy about taking the address of local variables,
> in order to determine stack location in C language. Instead, I think we
> should have AsmReadEsp() / AsmReadRsp() functions -- we used to have
> AsmReadSp() for Itanium. Please see the following sub-thread, where
> Jordan originally suggested AsmReadEsp() / AsmReadRsp():
> 
> http://mid.mail-archive.com/151056410867.15809.659701894226687543@jljusten-skl
> 
> http://mid.mail-archive.com/151059627258.20614.16505766191415005802@jljusten-skl
> 
> Should I file a Feature Request for BaseLib, about adding AsmReadEsp() /
> AsmReadRsp()?
> 
> I'm not suggesting that we block this patch with that feature request,
> but perhaps we should block the *next* patch.
> 
> 
> For the present patch, I'll follow up with test results separately.

I tested this patch on top of commit 4adf7074eb01. I found no regressions.

Assuming we only change the commit message of the patch (or not even that):

Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

If we change the patch due to (1) or (2) above, then I'd like to re-test
it; so please don't pick up my R-t-b for v2 in that case.

Thanks!
Laszlo

>> +
>> +  //
>> +  // If can't base on stack to find the AP index, use the APIC ID.
>> +  //
>> +  CurrentApicId = GetApicId ();
>> +  for (Index = 0; Index < TotalProcessorNumber; Index ++) {
>> +    if (CpuInfoInHob[Index].ApicId == CurrentApicId) {
>> +      *ProcessorNumber = Index;
>> +      return EFI_SUCCESS;
>> +    }
>> +  }
>> +
>>    return EFI_NOT_FOUND;
>>  }
>>
>> --
>> 2.15.0.windows.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>