[Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Eric Dong]

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 <ruiyu.ni@intel.com>
Cc: Jeff Fan <vanjeff_919@hotmail.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 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;
     }
   }
+
+  //
+  // 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

Re: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Dong, Eric]

Jeff,

Got it. Will update the change log when I commit the changes.

Thanks,
Eric
From: Fan Jeff [mailto:vanjeff_919@hotmail.com]
Sent: Wednesday, July 4, 2018 5:39 PM
To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.


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.



This code part is ok to me.

Reviewed-by: Jeff Fan vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>



Thanks!

Jeff



发送自 Windows 10 版邮件<https://go.microsoft.com/fwlink/?LinkId=550986>应用



________________________________
发件人: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
发送时间: Wednesday, July 4, 2018 4:37:36 PM
收件人: edk2-devel@lists.01.org<mailto: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 <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
Cc: Jeff Fan <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>>
Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
---
 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;
     }
   }
+
+  //
+  // 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

Re: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Laszlo Ersek]

Hi Eric,

On 07/05/18 03:26, Dong, Eric wrote:
> Jeff,
> 
> Got it. Will update the change log when I commit the changes.

please give me a bit more time to check this patch; I'll try to come to
it soon.

Thanks for your patience!
Laszlo

> 
> Thanks,
> Eric
> From: Fan Jeff [mailto:vanjeff_919@hotmail.com]
> Sent: Wednesday, July 4, 2018 5:39 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.
> 
> 
> 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.
> 
> 
> 
> This code part is ok to me.
> 
> Reviewed-by: Jeff Fan vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>
> 
> 
> 
> Thanks!
> 
> Jeff
> 
> 
> 
> 发送自 Windows 10 版邮件<https://go.microsoft.com/fwlink/?LinkId=550986>应用
> 
> 
> 
> ________________________________
> 发件人: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> 发送时间: Wednesday, July 4, 2018 4:37:36 PM
> 收件人: edk2-devel@lists.01.org<mailto: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 <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
> Cc: Jeff Fan <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> ---
>  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;
>      }
>    }
> +
> +  //
> +  // 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
> 

Re: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Laszlo Ersek]

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 <eric.dong@intel.com>
> 发送时间: 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 <ruiyu.ni@intel.com>
> Cc: Jeff Fan <vanjeff_919@hotmail.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  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
> 

Re: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Laszlo Ersek]

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 <eric.dong@intel.com>
>> 发送时间: 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 <ruiyu.ni@intel.com>
>> Cc: Jeff Fan <vanjeff_919@hotmail.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>> ---
>>  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 <lersek@redhat.com>

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
>>
> 

Re: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Fan Jeff]

Laszlo，

Thanks your remind. You are right! I agree original commit message is correct and neednt updating.

Eric, please ignore my comments on commit message.

Thanks!
Jeff



发自我的小米手机
在 Laszlo Ersek <lersek@redhat.com>，2018年7月5日 下午9:04写道：

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 <eric.dong@intel.com>
> 发送时间: 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 <ruiyu.ni@intel.com>
> Cc: Jeff Fan <vanjeff_919@hotmail.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  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
>

Re: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Dong, Eric]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, July 5, 2018 9:04 PM
> To: Fan Jeff <vanjeff_919@hotmail.com>; Dong, Eric <eric.dong@intel.com>;
> edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> 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 <eric.dong@intel.com>
> > 发送时间: 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 <ruiyu.ni@intel.com>
> > Cc: Jeff Fan <vanjeff_919@hotmail.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> >  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
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >

Re: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Dong, Eric]

Hi Laszlo,

I have created https://bugzilla.tianocore.org/show_bug.cgi?id=1002 to request to add AsmReadEsp() / AsmReadRsp().

Thanks,
Eric

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Dong, Eric
> Sent: Monday, July 9, 2018 11:04 AM
> To: Laszlo Ersek <lersek@redhat.com>; Fan Jeff <vanjeff_919@hotmail.com>;
> edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> processor number performance.
> 
> Hi Laszlo,
> 
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Thursday, July 5, 2018 9:04 PM
> > To: Fan Jeff <vanjeff_919@hotmail.com>; Dong, Eric
> > <eric.dong@intel.com>; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > 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 <eric.dong@intel.com>
> > > 发送时间: 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 <ruiyu.ni@intel.com>
> > > Cc: Jeff Fan <vanjeff_919@hotmail.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > > ---
> > >  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
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
> > >
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Laszlo Ersek]

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 <vanjeff_919@hotmail.com>; Dong, Eric <eric.dong@intel.com>;
>> edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>> 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 <eric.dong@intel.com>
>>> 发送时间: 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 <ruiyu.ni@intel.com>
>>> Cc: Jeff Fan <vanjeff_919@hotmail.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>>> ---
>>>  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
>>>
> 

Re: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Laszlo Ersek]

On 07/09/18 08:13, Dong, Eric wrote:
> Hi Laszlo,
> 
> I have created https://bugzilla.tianocore.org/show_bug.cgi?id=1002 to request to add AsmReadEsp() / AsmReadRsp().

Much appreciated!
Laszlo

Re: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Yao, Jiewen]

Hi
I believe using stack pointer is not a robust way if the stack guard feature is not enabled. Stack pointer may overflow.

Can we use GDT? Each AP has its own GDT.

Thank you
Yao Jiewen


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Dong,
> Eric
> Sent: Monday, July 9, 2018 2:13 PM
> To: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Fan
> Jeff <vanjeff_919@hotmail.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor
> number performance.
> 
> Hi Laszlo,
> 
> I have created https://bugzilla.tianocore.org/show_bug.cgi?id=1002 to request
> to add AsmReadEsp() / AsmReadRsp().
> 
> Thanks,
> Eric
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Dong, Eric
> > Sent: Monday, July 9, 2018 11:04 AM
> > To: Laszlo Ersek <lersek@redhat.com>; Fan Jeff <vanjeff_919@hotmail.com>;
> > edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> > processor number performance.
> >
> > Hi Laszlo,
> >
> > > -----Original Message-----
> > > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > > Sent: Thursday, July 5, 2018 9:04 PM
> > > To: Fan Jeff <vanjeff_919@hotmail.com>; Dong, Eric
> > > <eric.dong@intel.com>; edk2-devel@lists.01.org
> > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > > 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 <eric.dong@intel.com>
> > > > 发送时间: 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 <ruiyu.ni@intel.com>
> > > > Cc: Jeff Fan <vanjeff_919@hotmail.com>
> > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > > > ---
> > > >  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
> > > > edk2-devel@lists.01.org
> > > > https://lists.01.org/mailman/listinfo/edk2-devel
> > > >
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Dong, Eric]

Hi Jiewen,

I checked the code, found in the AP function (ApWakeupFunction), it updated the GDT value with the saved GDT value from BSP. So I think we can't use GDT in this case. Right?

      //
      // Sync BSP's Control registers to APs
      //
      RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);

Thanks,
Eric

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, July 11, 2018 3:45 PM
> To: Dong, Eric <eric.dong@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Fan Jeff <vanjeff_919@hotmail.com>;
> edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: RE: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> processor number performance.
> 
> Hi
> I believe using stack pointer is not a robust way if the stack guard feature is
> not enabled. Stack pointer may overflow.
> 
> Can we use GDT? Each AP has its own GDT.
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Dong, Eric
> > Sent: Monday, July 9, 2018 2:13 PM
> > To: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>; Fan Jeff <vanjeff_919@hotmail.com>;
> > edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> > processor number performance.
> >
> > Hi Laszlo,
> >
> > I have created https://bugzilla.tianocore.org/show_bug.cgi?id=1002 to
> > request to add AsmReadEsp() / AsmReadRsp().
> >
> > Thanks,
> > Eric
> >
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > > Of Dong, Eric
> > > Sent: Monday, July 9, 2018 11:04 AM
> > > To: Laszlo Ersek <lersek@redhat.com>; Fan Jeff
> > > <vanjeff_919@hotmail.com>; edk2-devel@lists.01.org
> > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> > > processor number performance.
> > >
> > > Hi Laszlo,
> > >
> > > > -----Original Message-----
> > > > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > > > Sent: Thursday, July 5, 2018 9:04 PM
> > > > To: Fan Jeff <vanjeff_919@hotmail.com>; Dong, Eric
> > > > <eric.dong@intel.com>; edk2-devel@lists.01.org
> > > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > > > 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 <eric.dong@intel.com>
> > > > > 发送时间: 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 <ruiyu.ni@intel.com>
> > > > > Cc: Jeff Fan <vanjeff_919@hotmail.com>
> > > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > > > > ---
> > > > >  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)))
> > > > > + CpuMpData->{
> > > > >        *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
> > > > > edk2-devel@lists.01.org
> > > > > https://lists.01.org/mailman/listinfo/edk2-devel
> > > > >
> > >
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

Re: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Laszlo Ersek]

On 07/11/18 13:31, Dong, Eric wrote:
> Hi Jiewen,
> 
> I checked the code, found in the AP function (ApWakeupFunction), it updated the GDT value with the saved GDT value from BSP. So I think we can't use GDT in this case. Right?
> 
>       //
>       // Sync BSP's Control registers to APs
>       //
>       RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);

Side remark: please remember that this particular chunk of code is
subject to change; due to the reviewed (but not yet committed) patch
from Ray:

  [PATCH] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP

  http://mid.mail-archive.com/20180702060135.264676-1-ruiyu.ni@intel.com

Said patch has *not* been committed yet, and *only* because Ray himself
has push rights, but he is currently away. So nobody has picked up the
patch yet.

I suggest that, before we do anything else for MpInitLib, we commit
Ray's patch first.

Eric, do you agree?

If so, can you push the patch, or do you want me to do it? I'm glad to
do it if you prefer.

Thanks,
Laszlo


>> -----Original Message-----
>> From: Yao, Jiewen
>> Sent: Wednesday, July 11, 2018 3:45 PM
>> To: Dong, Eric <eric.dong@intel.com>; Dong, Eric <eric.dong@intel.com>;
>> Laszlo Ersek <lersek@redhat.com>; Fan Jeff <vanjeff_919@hotmail.com>;
>> edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: RE: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
>> processor number performance.
>>
>> Hi
>> I believe using stack pointer is not a robust way if the stack guard feature is
>> not enabled. Stack pointer may overflow.
>>
>> Can we use GDT? Each AP has its own GDT.
>>
>> Thank you
>> Yao Jiewen
>>
>>
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>> Dong, Eric
>>> Sent: Monday, July 9, 2018 2:13 PM
>>> To: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
>>> <lersek@redhat.com>; Fan Jeff <vanjeff_919@hotmail.com>;
>>> edk2-devel@lists.01.org
>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>>> Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
>>> processor number performance.
>>>
>>> Hi Laszlo,
>>>
>>> I have created https://bugzilla.tianocore.org/show_bug.cgi?id=1002 to
>>> request to add AsmReadEsp() / AsmReadRsp().
>>>
>>> Thanks,
>>> Eric
>>>
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>>>> Of Dong, Eric
>>>> Sent: Monday, July 9, 2018 11:04 AM
>>>> To: Laszlo Ersek <lersek@redhat.com>; Fan Jeff
>>>> <vanjeff_919@hotmail.com>; edk2-devel@lists.01.org
>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>>>> Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
>>>> processor number performance.
>>>>
>>>> Hi Laszlo,
>>>>
>>>>> -----Original Message-----
>>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>>> Sent: Thursday, July 5, 2018 9:04 PM
>>>>> To: Fan Jeff <vanjeff_919@hotmail.com>; Dong, Eric
>>>>> <eric.dong@intel.com>; edk2-devel@lists.01.org
>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>>>>> 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 <eric.dong@intel.com>
>>>>>> 发送时间: 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 <ruiyu.ni@intel.com>
>>>>>> Cc: Jeff Fan <vanjeff_919@hotmail.com>
>>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>>>>>> ---
>>>>>>  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)))
>>>>>> + CpuMpData->{
>>>>>>        *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
>>>>>> edk2-devel@lists.01.org
>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>>>
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Dong, Eric]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, July 11, 2018 11:12 PM
> To: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Fan Jeff <vanjeff_919@hotmail.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> processor number performance.
> 
> On 07/11/18 13:31, Dong, Eric wrote:
> > Hi Jiewen,
> >
> > I checked the code, found in the AP function (ApWakeupFunction), it
> updated the GDT value with the saved GDT value from BSP. So I think we can't
> use GDT in this case. Right?
> >
> >       //
> >       // Sync BSP's Control registers to APs
> >       //
> >       RestoreVolatileRegisters
> > (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> 
> Side remark: please remember that this particular chunk of code is subject to
> change; due to the reviewed (but not yet committed) patch from Ray:
> 
>   [PATCH] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP
> 
>   http://mid.mail-archive.com/20180702060135.264676-1-ruiyu.ni@intel.com
> 
> Said patch has *not* been committed yet, and *only* because Ray himself
> has push rights, but he is currently away. So nobody has picked up the patch
> yet.
> 
> I suggest that, before we do anything else for MpInitLib, we commit Ray's
> patch first.
> 
> Eric, do you agree?
> 
> If so, can you push the patch, or do you want me to do it? I'm glad to do it if
> you prefer.
> 

Agree,  just push Ray's patch: SHA-1: c563077a380437c114aba4c95be65eb963ebc1f3

Let's continue.
 
 
> Thanks,
> Laszlo
> 
> 
> >> -----Original Message-----
> >> From: Yao, Jiewen
> >> Sent: Wednesday, July 11, 2018 3:45 PM
> >> To: Dong, Eric <eric.dong@intel.com>; Dong, Eric
> >> <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Fan Jeff
> >> <vanjeff_919@hotmail.com>; edk2-devel@lists.01.org
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> >> Subject: RE: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> >> processor number performance.
> >>
> >> Hi
> >> I believe using stack pointer is not a robust way if the stack guard
> >> feature is not enabled. Stack pointer may overflow.
> >>
> >> Can we use GDT? Each AP has its own GDT.
> >>
> >> Thank you
> >> Yao Jiewen
> >>
> >>
> >>> -----Original Message-----
> >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> >>> Of Dong, Eric
> >>> Sent: Monday, July 9, 2018 2:13 PM
> >>> To: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
> >>> <lersek@redhat.com>; Fan Jeff <vanjeff_919@hotmail.com>;
> >>> edk2-devel@lists.01.org
> >>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> >>> Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> >>> processor number performance.
> >>>
> >>> Hi Laszlo,
> >>>
> >>> I have created https://bugzilla.tianocore.org/show_bug.cgi?id=1002
> >>> to request to add AsmReadEsp() / AsmReadRsp().
> >>>
> >>> Thanks,
> >>> Eric
> >>>
> >>>> -----Original Message-----
> >>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> >>>> Of Dong, Eric
> >>>> Sent: Monday, July 9, 2018 11:04 AM
> >>>> To: Laszlo Ersek <lersek@redhat.com>; Fan Jeff
> >>>> <vanjeff_919@hotmail.com>; edk2-devel@lists.01.org
> >>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> >>>> Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> >>>> processor number performance.
> >>>>
> >>>> Hi Laszlo,
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >>>>> Sent: Thursday, July 5, 2018 9:04 PM
> >>>>> To: Fan Jeff <vanjeff_919@hotmail.com>; Dong, Eric
> >>>>> <eric.dong@intel.com>; edk2-devel@lists.01.org
> >>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> >>>>> 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 <eric.dong@intel.com>
> >>>>>> 发送时间: 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 <ruiyu.ni@intel.com>
> >>>>>> Cc: Jeff Fan <vanjeff_919@hotmail.com>
> >>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
> >>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>>>>> Signed-off-by: Eric Dong <eric.dong@intel.com>
> >>>>>> ---
> >>>>>>  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
> >>>>>> edk2-devel@lists.01.org
> >>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
> >>>>>>
> >>>>
> >>>> _______________________________________________
> >>>> edk2-devel mailing list
> >>>> edk2-devel@lists.01.org
> >>>> https://lists.01.org/mailman/listinfo/edk2-devel
> >>> _______________________________________________
> >>> edk2-devel mailing list
> >>> edk2-devel@lists.01.org
> >>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number performance.

[Dong, Eric]

Hi All,

As stack maybe overflow in some case and it will caused error AP index been returned. Also I don't find any register will be used different in each Aps. So I will update my patch to only base on APIC ID to get the AP.

Thanks,
Eric

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Dong, Eric
> Sent: Wednesday, July 11, 2018 7:32 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Fan Jeff <vanjeff_919@hotmail.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> processor number performance.
> 
> Hi Jiewen,
> 
> I checked the code, found in the AP function (ApWakeupFunction), it updated
> the GDT value with the saved GDT value from BSP. So I think we can't use GDT
> in this case. Right?
> 
>       //
>       // Sync BSP's Control registers to APs
>       //
>       RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters,
> FALSE);
> 
> Thanks,
> Eric
> 
> > -----Original Message-----
> > From: Yao, Jiewen
> > Sent: Wednesday, July 11, 2018 3:45 PM
> > To: Dong, Eric <eric.dong@intel.com>; Dong, Eric
> > <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Fan Jeff
> > <vanjeff_919@hotmail.com>; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > Subject: RE: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> > processor number performance.
> >
> > Hi
> > I believe using stack pointer is not a robust way if the stack guard
> > feature is not enabled. Stack pointer may overflow.
> >
> > Can we use GDT? Each AP has its own GDT.
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > > Of Dong, Eric
> > > Sent: Monday, July 9, 2018 2:13 PM
> > > To: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
> > > <lersek@redhat.com>; Fan Jeff <vanjeff_919@hotmail.com>;
> > > edk2-devel@lists.01.org
> > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> > > processor number performance.
> > >
> > > Hi Laszlo,
> > >
> > > I have created https://bugzilla.tianocore.org/show_bug.cgi?id=1002
> > > to request to add AsmReadEsp() / AsmReadRsp().
> > >
> > > Thanks,
> > > Eric
> > >
> > > > -----Original Message-----
> > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> > > > Behalf Of Dong, Eric
> > > > Sent: Monday, July 9, 2018 11:04 AM
> > > > To: Laszlo Ersek <lersek@redhat.com>; Fan Jeff
> > > > <vanjeff_919@hotmail.com>; edk2-devel@lists.01.org
> > > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > > > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get
> > > > processor number performance.
> > > >
> > > > Hi Laszlo,
> > > >
> > > > > -----Original Message-----
> > > > > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > > > > Sent: Thursday, July 5, 2018 9:04 PM
> > > > > To: Fan Jeff <vanjeff_919@hotmail.com>; Dong, Eric
> > > > > <eric.dong@intel.com>; edk2-devel@lists.01.org
> > > > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > > > > 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 <eric.dong@intel.com>
> > > > > > 发送时间: 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 <ruiyu.ni@intel.com>
> > > > > > Cc: Jeff Fan <vanjeff_919@hotmail.com>
> > > > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > > > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > > > > > ---
> > > > > >  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)
> > > > > > + CpuMpData->(&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
> > > > > > edk2-devel@lists.01.org
> > > > > > https://lists.01.org/mailman/listinfo/edk2-devel
> > > > > >
> > > >
> > > > _______________________________________________
> > > > edk2-devel mailing list
> > > > edk2-devel@lists.01.org
> > > > https://lists.01.org/mailman/listinfo/edk2-devel
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel