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

["Dong, Eric" <eric.dong@intel.com>]

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