From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3CB41207DF28D for ; Thu, 5 Jul 2018 06:04:14 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 737A64067720; Thu, 5 Jul 2018 13:04:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-21.rdu2.redhat.com [10.10.120.21]) by smtp.corp.redhat.com (Postfix) with ESMTP id 81BE17C3F; Thu, 5 Jul 2018 13:04:12 +0000 (UTC) To: Fan Jeff , Eric Dong , "edk2-devel@lists.01.org" Cc: Ruiyu Ni References: <20180704083736.9272-1-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <58bddde7-da02-1a73-f738-70c58c10b60a@redhat.com> Date: Thu, 5 Jul 2018 15:04:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 05 Jul 2018 13:04:13 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 05 Jul 2018 13:04:13 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: =?UTF-8?B?UmU6IOetlOWkjTogW1BhdGNoXSBVZWZpQ3B1UGtnL01wSW5pdExpYjogT3B0aW1pemUgZ2V0IHByb2Nlc3NvciBudW1iZXIgcGVyZm9ybWFuY2Uu?= X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Jul 2018 13:04:14 -0000 Content-Type: text/plain; charset=UTF-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 > 发送时间: 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 > Cc: Jeff Fan > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > 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. 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 >