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

[Laszlo Ersek <lersek@redhat.com>]

Hi Eric,

On 07/05/18 03:26, Dong, Eric wrote:
> Jeff,
> 
> Got it. Will update the change log when I commit the changes.

please give me a bit more time to check this patch; I'll try to come to
it soon.

Thanks for your patience!
Laszlo

> 
> Thanks,
> Eric
> From: Fan Jeff [mailto:vanjeff_919@hotmail.com]
> Sent: Wednesday, July 4, 2018 5:39 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.
> 
> 
> 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.
> 
> 
> 
> This code part is ok to me.
> 
> Reviewed-by: Jeff Fan vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>
> 
> 
> 
> Thanks!
> 
> Jeff
> 
> 
> 
> 发送自 Windows 10 版邮件<https://go.microsoft.com/fwlink/?LinkId=550986>应用
> 
> 
> 
> ________________________________
> 发件人: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> 发送时间: Wednesday, July 4, 2018 4:37:36 PM
> 收件人: edk2-devel@lists.01.org<mailto: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<mailto:ruiyu.ni@intel.com>>
> Cc: Jeff Fan <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com<mailto: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;
>      }
>    }
> +
> +  //
> +  // 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
>