Hi Laszlo, > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Thursday, July 5, 2018 9:04 PM > To: Fan Jeff <[email protected]>; Dong, Eric <[email protected]>; > [email protected] > Cc: Ni, Ruiyu <[email protected]> > 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 <[email protected]> > > 发送时间: Wednesday, July 4, 2018 4:37:36 PM > > 收件人: [email protected] > > 抄送: 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 <[email protected]> > > Cc: Jeff Fan <[email protected]> > > Cc: Laszlo Ersek <[email protected]> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Eric Dong <[email protected]> > > --- > > 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 > > [email protected] > > https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
