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 0457D209688F3 for ; Thu, 5 Jul 2018 06:15:42 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F10C5407CCD8; Thu, 5 Jul 2018 13:15:41 +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 260C3215688A; Thu, 5 Jul 2018 13:15:40 +0000 (UTC) From: Laszlo Ersek To: Fan Jeff , Eric Dong , "edk2-devel@lists.01.org" Cc: Ruiyu Ni References: <20180704083736.9272-1-eric.dong@intel.com> <58bddde7-da02-1a73-f738-70c58c10b60a@redhat.com> Message-ID: <31b0c9f5-0bb1-e21e-0614-f65fd4acb8ad@redhat.com> Date: Thu, 5 Jul 2018 15:15:40 +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: <58bddde7-da02-1a73-f738-70c58c10b60a@redhat.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 05 Jul 2018 13:15:42 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 05 Jul 2018 13:15:42 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.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:15:43 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 >> 发送时间: 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. 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 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 >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel >> >