Re: [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State.

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

Hi Laszlo,


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, July 25, 2018 6:55 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> 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 <lersek@redhat.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> >  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:
> 
>   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 (&CpuMpData->CpuData[ProcessorNumber],
> CpuStateFinished);
> > +        SetApState (&CpuMpData->CpuData[ProcessorNumber],
> > + CpuStateIdle);
> >        }
> >      }
> >
> > @@ -1352,18 +1352,17 @@ CheckThisAP (
> >    CpuData   = &CpuMpData->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 = &CpuMpData->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.
> 

Yes, we have two flags here.  Waiting flags means the AP will do the task. State flag means whether the task has finished.
Both flags will be checked and updated.

> I'll follow up with test results.
> 
> Thanks,
> Laszlo
> 
> > @@ -1923,7 +1921,7 @@ SwitchBSPWorker (
> >    //
> >    // Wait for old BSP finished AP task
> >    //
> > -  while (GetApState (&CpuMpData->CpuData[CallerNumber]) !=
> > CpuStateFinished) {
> > +  while (GetApState (&CpuMpData->CpuData[CallerNumber]) !=
> > + CpuStateIdle) {
> >      CpuPause ();
> >    }
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > index 9d0b866d09..962bce685d 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > @@ -85,7 +85,6 @@ typedef enum {
> >    CpuStateIdle,
> >    CpuStateReady,
> >    CpuStateBusy,
> > -  CpuStateFinished,
> >    CpuStateDisabled
> >  } CPU_STATE;
> >
> >