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

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

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, July 5, 2018 9:04 PM
> To: Fan Jeff <vanjeff_919@hotmail.com>; Dong, Eric <eric.dong@intel.com>;
> edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> processor number performance.
> 
> 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 "<="?
> 

[Eric]  TotalProcessorNumber is the first local variable in this function, also exist other local variables in this function, so I just use "<" here.

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

[Eric] Yes, I tries to use the function you suggested but we don't find it, so I use local variable here.  I agree with your suggest that we should add this API for later usage. I will follow up to add this new API and update this patch to V2.

> 
> For the present patch, I'll follow up with test results separately.
> 
> 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
> >