From: "Ni, Ray" <ray.ni@intel.com>
To: "Zeng, Star" <star.zeng@intel.com>,
"Wu, Hao A" <hao.a.wu@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Dong, Eric" <eric.dong@intel.com>,
Laszlo Ersek <lersek@redhat.com>,
"Kinney, Michael D" <michael.d.kinney@intel.com>,
"Brian J . Johnson" <brian.johnson@hpe.com>
Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval
Date: Tue, 24 Mar 2020 07:52:39 +0000 [thread overview]
Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C4AA5E3@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB048310405D411B@shsmsx102.ccr.corp.intel.com>
>From the perspective of PCD producer, I agree with Star to use UINT32.
Internal implementation can be changed to use other polling methods which don't require UINT64 data type.
> -----Original Message-----
> From: Zeng, Star <star.zeng@intel.com>
> Sent: Tuesday, March 24, 2020 3:29 PM
> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Brian J . Johnson <brian.johnson@hpe.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval
>
> > -----Original Message-----
> > From: Wu, Hao A
> > Sent: Tuesday, March 24, 2020 3:12 PM
> > To: Zeng, Star <star.zeng@intel.com>; devel@edk2.groups.io
> > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> > Ersek <lersek@redhat.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Brian J . Johnson <brian.johnson@hpe.com>
> > Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to
> > control AP status check interval
> >
> > > -----Original Message-----
> > > From: Zeng, Star
> > > Sent: Tuesday, March 24, 2020 3:02 PM
> > > To: devel@edk2.groups.io; Wu, Hao A
> > > Cc: Dong, Eric; Ni, Ray; Laszlo Ersek; Kinney, Michael D; Brian J .
> > > Johnson; Zeng, Star
> > > Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD
> > > to control AP status check interval
> > >
> > > The code logic is good to me.
> > > Only minor concern, do we really need the PCD to be UINT64 type? :)
> >
> >
> > Hi Star,
> >
> > I am thinking the UINT64 type fits with the parameter for the SetTimer()
> > service in EFI_BOOT_SERVICES when preparing the patch.
> >
> > If there is concern, I can switch it to other type.
>
> Got your good point.
> For me, UINT32 (FFFFFFFFh = 4294967295d = 4294967.295 second = 71582.78825 minutes ~=
> 1193.0464708333333333333333333333 hours) should be totally enough.
> I do not insist on that if others think UINT64 is ok.
>
> Thanks,
> Star
>
> >
> > Best Regards,
> > Hao Wu
> >
> >
> > >
> > > Thanks,
> > > Star
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> > > > Of Wu, Hao A
> > > > Sent: Tuesday, March 24, 2020 2:33 PM
> > > > To: devel@edk2.groups.io
> > > > Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric
> > > > <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> > > > <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> > Zeng, Star <star.zeng@intel.com>; Brian J .
> > > > Johnson <brian.johnson@hpe.com>
> > > > Subject: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD
> > > > to control AP status check interval
> > > >
> > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2627
> > > >
> > > > The commit will introduce a static PCD to specify the periodic
> > > > interval for checking the AP status when MP services StartupAllAPs()
> > > > and
> > > > StartupThisAP() are being executed in a non-blocking manner. Or in
> > > > other words, specifies the interval for callback function CheckApsStatus().
> > > >
> > > > The purpose is to provide the platform owners with the ability to
> > > > choose the proper interval value to trigger CheckApsStatus() according to:
> > > > A) The number of processors in the system;
> > > > B) How MP services (StartupAllAPs & StartupThisAP) being used.
> > > >
> > > > Setting the PCD to a small value means the AP status check callback
> > > > will be trigerred more frequently, it can benefit the performance
> > > > for the case when the BSP uses WaitForEvent() or uses CheckEvent()
> > > > in a loop to wait for AP(s) to complete the task, especially when
> > > > the task can be finished considerably fast on AP(s).
> > > >
> > > > An example is within function CpuFeaturesInitialize() under
> > > > UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.
> > > > c, where BSP will perform the same task with APs and requires all
> > > > the processors to finish the task before BSP proceeds to its next
> > > > task.
> > > >
> > > > Setting the PCD to a big value, on the other hand, can reduce the
> > > > impact on BSP by the time being consumed in CheckApsStatus(),
> > > > especially when the number of processors is huge so that the time
> > > > consumed in CheckApsStatus() is not negligible.
> > > >
> > > > For least impact, the default value of the new PCD will be the same
> > > > with the current interval value, which is 100 milliseconds.
> > > >
> > > > Unitest done:
> > > > OS boot successfully.
> > > >
> > > > Cc: Eric Dong <eric.dong@intel.com>
> > > > Cc: Ray Ni <ray.ni@intel.com>
> > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > Cc: Star Zeng <star.zeng@intel.com>
> > > > Cc: Brian J. Johnson <brian.johnson@hpe.com>
> > > > Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> > > > ---
> > > > UefiCpuPkg/UefiCpuPkg.dec | 6 ++++++
> > > > UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 2 +-
> > > > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 3 +--
> > > > UefiCpuPkg/UefiCpuPkg.uni | 5 ++++-
> > > > 4 files changed, 12 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> > > > index
> > > > e91dc68cbe..06a3d52060 100644
> > > > --- a/UefiCpuPkg/UefiCpuPkg.dec
> > > > +++ b/UefiCpuPkg/UefiCpuPkg.dec
> > > > @@ -230,6 +230,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
> > > > # @Prompt This PCD is the nominal frequency of the core crystal
> > > > clock in Hz as is CPUID Leaf 0x15:ECX
> > > >
> > > >
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency|24000000|
> > > > UINT64|0x32132113
> > > >
> > > > + ## Specifies the periodic interval value in milliseconds for the
> > > > + status check # of APs for StartupAllAPs() and StartupThisAP()
> > > > + executed in non-blocking # mode in DXE phase.
> > > > + # @Prompt Periodic interval value in milliseconds for AP status
> > > > + check in
> > > > DXE.
> > > > +
> > > > +
> > > >
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval|100|UINT64|0x
> > > > 000
> > > > + 0001E
> > > > +
> > > > [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic,
> > PcdsDynamicEx]
> > > > ## Specifies max supported number of Logical Processors.
> > > > # @Prompt Configure max supported number of Logical Processors
> > > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > > b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > > index 45aaa179ff..05e004dc3c 100644
> > > > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > > @@ -69,5 +69,5 @@ [Pcd]
> > > > gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize ##
> > > > CONSUMES
> > > > gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode ##
> > > > CONSUMES
> > > > gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate ##
> > > > SOMETIMES_CONSUMES
> > > > + gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval ##
> > > > CONSUMES
> > > > gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ##
> > > > CONSUMES
> > > > -
> > > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > index a987c32109..683547dc99 100644
> > > > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > @@ -15,7 +15,6 @@
> > > >
> > > > #include <Protocol/Timer.h>
> > > >
> > > > -#define AP_CHECK_INTERVAL (EFI_TIMER_PERIOD_MILLISECONDS
> > (100))
> > > > #define AP_SAFE_STACK_SIZE 128
> > > >
> > > > CPU_MP_DATA *mCpuMpData = NULL;
> > > > @@ -451,7 +450,7 @@ InitMpGlobalData (
> > > > Status = gBS->SetTimer (
> > > > mCheckAllApsEvent,
> > > > TimerPeriodic,
> > > > - AP_CHECK_INTERVAL
> > > > + PcdGet64 (PcdCpuApStatusCheckInterval)
> > > > );
> > > > ASSERT_EFI_ERROR (Status);
> > > >
> > > > diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> > > > index
> > > > c0d6ed5136..08d9b45f76 100644
> > > > --- a/UefiCpuPkg/UefiCpuPkg.uni
> > > > +++ b/UefiCpuPkg/UefiCpuPkg.uni
> > > > @@ -3,7 +3,7 @@
> > > > //
> > > > // This Package provides UEFI compatible CPU modules and libraries.
> > > > //
> > > > -// Copyright (c) 2007 - 2015, Intel Corporation. All rights
> > > > reserved.<BR>
> > > > +// Copyright (c) 2007 - 2020, Intel Corporation. All rights
> > > > +reserved.<BR>
> > > > //
> > > > // SPDX-License-Identifier: BSD-2-Clause-Patent // @@ -275,3
> > > > +275,6 @@ #string
> > > >
> > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_PR
> > > > OMPT #language en-US "Specify the count of pre allocated SMM MP
> > > > tokens per chunk.\n"
> > > >
> > > > #string
> > > >
> > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_HE
> > > > LP #language en-US "This value used to specify the count of pre
> > allocated
> > > > SMM MP tokens per chunk.\n"
> > > > +
> > > > +#string
> > > >
> > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterval_PROMPT
> > > > #language en-US "Periodic interval value in milliseconds for AP
> > > > status check in DXE.\n"
> > > > +#string
> > > > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterval_HELP
> > > > #language en-US "Periodic interval value in milliseconds for the
> > > > status check of APs for StartupAllAPs() and StartupThisAP() executed
> > > > in non-blocking mode in DXE phase.\n"
> > > > --
> > > > 2.12.0.windows.1
> > > >
> > > >
> > > >
next prev parent reply other threads:[~2020-03-24 7:52 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-24 6:33 [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval Wu, Hao A
2020-03-24 7:01 ` [edk2-devel] " Zeng, Star
2020-03-24 7:11 ` Wu, Hao A
2020-03-24 7:29 ` Zeng, Star
2020-03-24 7:52 ` Ni, Ray [this message]
2020-03-24 15:59 ` Michael D Kinney
2020-03-25 0:46 ` Ni, Ray
2020-03-25 1:24 ` Wu, Hao A
2020-03-25 16:33 ` Laszlo Ersek
2020-03-25 1:37 ` Laszlo Ersek
2020-03-25 1:50 ` Wu, Hao A
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=734D49CCEBEEF84792F5B80ED585239D5C4AA5E3@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox