Re: [edk2] [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State.
Eric, Please also include the state machine in comments for ENUM CPU_STATE definition. Reviewed-by: Ruiyu Ni Thanks/Ray > -Original Message- > From: Dong, Eric > Sent: Wednesday, July 25, 2018 7:15 PM > To: Laszlo Ersek ; edk2-devel@lists.01.org > Cc: Ni, Ruiyu > Subject: RE: [edk2] [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant > CpuStateFinished State. > > Hi Laszlo, > > > > -Original Message- > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > Sent: Wednesday, July 25, 2018 6:55 PM > > To: Dong, Eric ; edk2-devel@lists.01.org > > Cc: Ni, Ruiyu > > Subject: Re: [edk2] [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove > > redundant CpuStateFinished State. > > > > Hi Eric, > > > > On 07/25/18 09:50, Eric Dong wrote: > > > Current CPU state definition include CpuStateIdle and CpuStateFinished. > > > After investigation, current code can use CpuStateIdle to replace > > > the CpuStateFinished. It will reduce the state number and easy for > maintenance. > > > > > > Cc: Laszlo Ersek > > > Cc: Ruiyu Ni > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Eric Dong > > > --- > > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 18 -- > > > UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 - > > > 2 files changed, 8 insertions(+), 11 deletions(-) > > > > After looking over this patch, it seems that you are preserving the > > CpuStateReady enum constant, relative to: > > > > > > 20180628112920.5296-1-eric.dong@intel.com">http://mid.mail-archive.com/20180628112920.5296-1-eric.dong@intel.com > > > > However, based on your analysis in > > > > http://mid.mail- > > archive.com/ED077930C258884BBCB450DB737E66224AC5A453@shsmsx102. > > ccr.corp.intel.com > > > > isn't it still possible to run into the exact same issue? (Namely, BSP > > thinks the AP has gone through Idle -> Busy -> Idle, but the AP has > > never actually left > > Idle?) > > > > Hm, wait, is it the case that the BSP first sets Ready, and so if the > > check for an AP returns Idle, it implies the AP must have gone through: > > > > Idle > Ready > Busy > Idle > > > > ? > > Correct! The Ready state is begin state and the Idle is the finish state. > > > > > If this is correct, can you please include the following in the commit > > message: > > > > > Before this patch, the state transitions for an AP are: > > > > > > Idle > Ready > Busy > Finished > Idle > > >[BSP] [AP] [AP] [BSP] > > > > > > After the patch, the state transitions for an AP are: > > > > > > Idle > Ready > Busy > Idle > > >[BSP] [AP] [AP] > > > > Do you agree? > > Good suggestion, I will include this info in the commit message. > > > > > I have another question: > > > > On 07/25/18 09:50, Eric Dong wrote: > > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > index c82b985943..ff09a0e9e7 100644 > > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > @@ -696,7 +696,7 @@ ApWakeupFunction ( > > > } > > >} > > > } > > > -SetApState (>CpuData[ProcessorNumber], > > CpuStateFinished); > > > +SetApState (>CpuData[ProcessorNumber], > > > + CpuStateIdle); > > >} > > > } > > > > > > @@ -1352,18 +1352,17 @@ CheckThisAP ( > > >CpuData = >CpuData[ProcessorNumber]; > > > > > >// > > > - // Check the CPU state of AP. If it is CpuStateFinished, then > > > the AP has > > finished its task. > > > + // Check the CPU state of AP. If it is CpuStateIdle, then the AP > > > + has > > finished its task. > > >// Only BSP and corresponding AP access this unit of CPU Data. > > > This means the AP will not modify the > > > - // value of state after setting the it to CpuStateFinished, so > > > BSP can safely > > make use of its value. > > > + // value of state after setting the it to CpuStateIdle, so BSP > > > + can safely > > make use of its value. > > >// > > >// > > >// If the
Re: [edk2] [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State.
Hi Laszlo,
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, July 25, 2018 6:55 PM
> To: Dong, Eric ; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu
> Subject: Re: [edk2] [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant
> CpuStateFinished State.
>
> Hi Eric,
>
> On 07/25/18 09:50, Eric Dong wrote:
> > Current CPU state definition include CpuStateIdle and CpuStateFinished.
> > After investigation, current code can use CpuStateIdle to replace the
> > CpuStateFinished. It will reduce the state number and easy for maintenance.
> >
> > Cc: Laszlo Ersek
> > Cc: Ruiyu Ni
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong
> > ---
> > UefiCpuPkg/Library/MpInitLib/MpLib.c | 18 --
> > UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 -
> > 2 files changed, 8 insertions(+), 11 deletions(-)
>
> After looking over this patch, it seems that you are preserving the
> CpuStateReady enum constant, relative to:
>
> 20180628112920.5296-1-eric.dong@intel.com">http://mid.mail-archive.com/20180628112920.5296-1-eric.dong@intel.com
>
> However, based on your analysis in
>
> http://mid.mail-
> archive.com/ED077930C258884BBCB450DB737E66224AC5A453@shsmsx102.
> ccr.corp.intel.com
>
> isn't it still possible to run into the exact same issue? (Namely, BSP thinks
> the
> AP has gone through Idle -> Busy -> Idle, but the AP has never actually left
> Idle?)
>
> Hm, wait, is it the case that the BSP first sets Ready, and so if the check
> for an
> AP returns Idle, it implies the AP must have gone through:
>
> Idle > Ready > Busy > Idle
>
> ?
Correct! The Ready state is begin state and the Idle is the finish state.
>
> If this is correct, can you please include the following in the commit
> message:
>
> > Before this patch, the state transitions for an AP are:
> >
> > Idle > Ready > Busy > Finished > Idle
> >[BSP] [AP] [AP] [BSP]
> >
> > After the patch, the state transitions for an AP are:
> >
> > Idle > Ready > Busy > Idle
> >[BSP] [AP] [AP]
>
> Do you agree?
Good suggestion, I will include this info in the commit message.
>
> I have another question:
>
> On 07/25/18 09:50, Eric Dong wrote:
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index c82b985943..ff09a0e9e7 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -696,7 +696,7 @@ ApWakeupFunction (
> > }
> >}
> > }
> > -SetApState (>CpuData[ProcessorNumber],
> CpuStateFinished);
> > +SetApState (>CpuData[ProcessorNumber],
> > + CpuStateIdle);
> >}
> > }
> >
> > @@ -1352,18 +1352,17 @@ CheckThisAP (
> >CpuData = >CpuData[ProcessorNumber];
> >
> >//
> > - // Check the CPU state of AP. If it is CpuStateFinished, then the AP has
> finished its task.
> > + // Check the CPU state of AP. If it is CpuStateIdle, then the AP has
> finished its task.
> >// Only BSP and corresponding AP access this unit of CPU Data.
> > This means the AP will not modify the
> > - // value of state after setting the it to CpuStateFinished, so BSP can
> > safely
> make use of its value.
> > + // value of state after setting the it to CpuStateIdle, so BSP can
> > safely
> make use of its value.
> >//
> >//
> >// If the AP finishes for StartupThisAP(), return EFI_SUCCESS.
> >//
> > - if (GetApState(CpuData) == CpuStateFinished) {
> > + if (GetApState(CpuData) == CpuStateIdle) {
> > if (CpuData->Finished != NULL) {
> >*(CpuData->Finished) = TRUE;
> > }
> > -SetApState (CpuData, CpuStateIdle);
> > return EFI_SUCCESS;
> >} else {
> > //
> > @@ -1420,14 +1419,13 @@ CheckAllAPs (
> >
> > CpuData = >CpuData[ProcessorNumber];
> > //
> > -// Check the CPU state of AP. If it is CpuStateFinished, then the AP
> > has
> finished its task.
> > +// Check the CPU state of AP. If it is CpuStateIdle, then the AP has
> finished its task.
> > // Only BSP and corresponding AP access this unit of CPU Data. This
> means the AP will not modify the
> > -// value of state after setting the it to CpuStateFinished,
Re: [edk2] [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State.
Hi Eric,
On 07/25/18 09:50, Eric Dong wrote:
> Current CPU state definition include CpuStateIdle and CpuStateFinished.
> After investigation, current code can use CpuStateIdle to replace the
> CpuStateFinished. It will reduce the state number and easy for maintenance.
>
> Cc: Laszlo Ersek
> Cc: Ruiyu Ni
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 18 --
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 -
> 2 files changed, 8 insertions(+), 11 deletions(-)
After looking over this patch, it seems that you are preserving the
CpuStateReady enum constant, relative to:
20180628112920.5296-1-eric.dong@intel.com">http://mid.mail-archive.com/20180628112920.5296-1-eric.dong@intel.com
However, based on your analysis in
ED077930C258884BBCB450DB737E66224AC5A453@shsmsx102.ccr.corp.intel.com">http://mid.mail-archive.com/ED077930C258884BBCB450DB737E66224AC5A453@shsmsx102.ccr.corp.intel.com
isn't it still possible to run into the exact same issue? (Namely, BSP
thinks the AP has gone through Idle -> Busy -> Idle, but the AP has
never actually left Idle?)
Hm, wait, is it the case that the BSP first sets Ready, and so if the
check for an AP returns Idle, it implies the AP must have gone through:
Idle > Ready > Busy > Idle
?
If this is correct, can you please include the following in the commit
message:
> Before this patch, the state transitions for an AP are:
>
> Idle > Ready > Busy > Finished > Idle
>[BSP] [AP] [AP] [BSP]
>
> After the patch, the state transitions for an AP are:
>
> Idle > Ready > Busy > Idle
>[BSP] [AP] [AP]
Do you agree?
I have another question:
On 07/25/18 09:50, Eric Dong wrote:
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index c82b985943..ff09a0e9e7 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -696,7 +696,7 @@ ApWakeupFunction (
> }
>}
> }
> -SetApState (>CpuData[ProcessorNumber], CpuStateFinished);
> +SetApState (>CpuData[ProcessorNumber], CpuStateIdle);
>}
> }
>
> @@ -1352,18 +1352,17 @@ CheckThisAP (
>CpuData = >CpuData[ProcessorNumber];
>
>//
> - // Check the CPU state of AP. If it is CpuStateFinished, then the AP has
> finished its task.
> + // Check the CPU state of AP. If it is CpuStateIdle, then the AP has
> finished its task.
>// Only BSP and corresponding AP access this unit of CPU Data. This means
> the AP will not modify the
> - // value of state after setting the it to CpuStateFinished, so BSP can
> safely make use of its value.
> + // value of state after setting the it to CpuStateIdle, so BSP can safely
> make use of its value.
>//
>//
>// If the AP finishes for StartupThisAP(), return EFI_SUCCESS.
>//
> - if (GetApState(CpuData) == CpuStateFinished) {
> + if (GetApState(CpuData) == CpuStateIdle) {
> if (CpuData->Finished != NULL) {
>*(CpuData->Finished) = TRUE;
> }
> -SetApState (CpuData, CpuStateIdle);
> return EFI_SUCCESS;
>} else {
> //
> @@ -1420,14 +1419,13 @@ CheckAllAPs (
>
> CpuData = >CpuData[ProcessorNumber];
> //
> -// Check the CPU state of AP. If it is CpuStateFinished, then the AP has
> finished its task.
> +// Check the CPU state of AP. If it is CpuStateIdle, then the AP has
> finished its task.
> // Only BSP and corresponding AP access this unit of CPU Data. This
> means the AP will not modify the
> -// value of state after setting the it to CpuStateFinished, so BSP can
> safely make use of its value.
> +// value of state after setting the it to CpuStateIdle, so BSP can
> safely make use of its value.
> //
> -if (GetApState(CpuData) == CpuStateFinished) {
> +if (GetApState(CpuData) == CpuStateIdle) {
>CpuMpData->RunningCount ++;
>CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
> - SetApState(CpuData, CpuStateIdle);
>
>//
>// If in Single Thread mode, then search for the next waiting AP for
> execution.
This part of the code, after the patch, does not seem idempotent; in
other words, if the BSP calls CheckAllAPs() multiple times, then
RunningCount will be increased every time. Before the patch, this wasn't
the case, because after the Finished -> Idle transition, the increment
wouldn't be reached again.
Hmmm, wait, I'm wrong: we set the Waiting field to FALSE as well, so at
the next call to CheckAllAPs(), we'll take the early "continue" branch.
Looks OK after all.
I'll follow up with test results.
Thanks,
Laszlo
> @@ -1923,7 +1921,7 @@ SwitchBSPWorker (
>//
>// Wait for old BSP finished AP task
>//
> - while (GetApState (>CpuData[CallerNumber]) != CpuStateFinished)
> {
> + while (GetApState
