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 B27AC202E53E4 for ; Mon, 9 Jul 2018 01:47:50 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EF654401C7E9; Mon, 9 Jul 2018 08:47:49 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-21.rdu2.redhat.com [10.10.121.21]) by smtp.corp.redhat.com (Postfix) with ESMTP id EEDCA111DCE7; Mon, 9 Jul 2018 08:47:48 +0000 (UTC) To: "Dong, Eric" , Fan Jeff , "edk2-devel@lists.01.org" Cc: "Ni, Ruiyu" References: <20180704083736.9272-1-eric.dong@intel.com> <58bddde7-da02-1a73-f738-70c58c10b60a@redhat.com> From: Laszlo Ersek Message-ID: Date: Mon, 9 Jul 2018 10:47:48 +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.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Mon, 09 Jul 2018 08:47:50 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Mon, 09 Jul 2018 08:47:50 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.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.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Jul 2018 08:47:52 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 07/09/18 05:04, Dong, Eric wrote: > Hi Laszlo, > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, July 5, 2018 9:04 PM >> To: Fan Jeff ; Dong, Eric ; >> edk2-devel@lists.01.org >> Cc: Ni, Ruiyu >> 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 >>> 发送时间: 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 "<="? >> > > [Eric] TotalProcessorNumber is the first local variable in this function, also exist other local variables in this function, so I just use "<" here. Unfortunately, this argument does not work in GCC builds. The ISO C standard does not say anything about the addresses of local variables, and indeed GCC occasionally rearranges local variables between each other. Please see commit f98f5ec304ec ("UefiCpuPkg: S3Resume2Pei: align return stacks explicitly", 2013-12-13) as an example. >> (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. Oh, that's great! Thank you! Laszlo > >> >> 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 >>> >