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 <[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 "<="? > > > (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 <[email protected]> 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 >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel >> > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
