Re: [edk2] [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.

2018-07-25 Thread Ni, Ruiyu 
Reviewed-by: Ruiyu Ni 

Thanks/Ray

> -Original Message-
> From: edk2-devel  On Behalf Of Eric Dong
> Sent: Wednesday, July 25, 2018 3:50 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu ; Laszlo Ersek 
> Subject: [edk2] [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and
> volatile definition.
> 
> The StartCount is duplicated with RunningCount, replace it with RunningCount.
> Also the volatile for RunningCount is not needed.
> 
> Cc: Laszlo Ersek 
> Cc: Ruiyu Ni 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong 
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 +--
> UefiCpuPkg/Library/MpInitLib/MpLib.h |  3 +--
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index ff09a0e9e7..0e57cc86bf 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1424,7 +1424,7 @@ CheckAllAPs (
>  // value of state after setting the it to CpuStateIdle, so BSP can 
> safely make
> use of its value.
>  //
>  if (GetApState(CpuData) == CpuStateIdle) {
> -  CpuMpData->RunningCount ++;
> +  CpuMpData->RunningCount --;
>CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
> 
>//
> @@ -1449,7 +1449,7 @@ CheckAllAPs (
>//
>// If all APs finish, return EFI_SUCCESS.
>//
> -  if (CpuMpData->RunningCount == CpuMpData->StartCount) {
> +  if (CpuMpData->RunningCount == 0) {
>  return EFI_SUCCESS;
>}
> 
> @@ -1466,7 +1466,7 @@ CheckAllAPs (
>  //
>  if (CpuMpData->FailedCpuList != NULL) {
>*CpuMpData->FailedCpuList =
> - AllocatePool ((CpuMpData->StartCount - CpuMpData->FinishedCount + 1)
> * sizeof (UINTN));
> + AllocatePool ((CpuMpData->RunningCount + 1) * sizeof (UINTN));
>ASSERT (*CpuMpData->FailedCpuList != NULL);
>  }
>  ListIndex = 0;
> @@ -2212,7 +2212,7 @@ StartupAllAPsWorker (
>  return EFI_NOT_STARTED;
>}
> 
> -  CpuMpData->StartCount = 0;
> +  CpuMpData->RunningCount = 0;
>for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount;
> ProcessorNumber++) {
>  CpuData = >CpuData[ProcessorNumber];
>  CpuData->Waiting = FALSE;
> @@ -,7 +,7 @@ StartupAllAPsWorker (
>  // Mark this processor as responsible for current calling.
>  //
>  CpuData->Waiting = TRUE;
> -CpuMpData->StartCount++;
> +CpuMpData->RunningCount++;
>}
>  }
>}
> @@ -2231,7 +2231,6 @@ StartupAllAPsWorker (
>CpuMpData->ProcArguments = ProcedureArgument;
>CpuMpData->SingleThread  = SingleThread;
>CpuMpData->FinishedCount = 0;
> -  CpuMpData->RunningCount  = 0;
>CpuMpData->FailedCpuList = FailedCpuList;
>CpuMpData->ExpectedTime  = CalculateTimeout (
> TimeoutInMicroseconds, diff --git
> a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 962bce685d..5002b7e9c0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -211,9 +211,8 @@ struct _CPU_MP_DATA {
>UINTN  BackupBuffer;
>UINTN  BackupBufferSize;
> 
> -  volatile UINT32StartCount;
>volatile UINT32FinishedCount;
> -  volatile UINT32RunningCount;
> +  UINT32 RunningCount;
>BOOLEANSingleThread;
>EFI_AP_PROCEDURE   Procedure;
>VOID   *ProcArguments;
> --
> 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: [edk2] [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.

2018-07-25 Thread Laszlo Ersek 
On 07/25/18 14:09, Dong, Eric wrote:
> Hi Laszlo,
> 
> 
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Wednesday, July 25, 2018 7:47 PM
>> To: Dong, Eric ; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu 
>> Subject: Re: [edk2] [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount
>> and volatile definition.
>>
>> Hi Eric,
>>
>> On 07/25/18 09:50, Eric Dong wrote:
>>> The StartCount is duplicated with RunningCount, replace it with
>>> RunningCount. Also the volatile for RunningCount is not needed.
>>
>> after staring at this patch for a long time, I think it is correct.
>> However, I suggest that we improve the commit message, because this patch
>> actually does three things:
>>
>> (1) It removes "volatile" from RunningCount.
>>
>> [This is OK, because only the BSP modifies it.]
>>
>> (2) [This is the tricky part.]
>>
>> When we detect a timeout in CheckAllAPs(), and collect the list of failed 
>> CPUs,
>> the size of the list is derived from the following difference, before the 
>> patch:
>>
>>   StartCount - FinishedCount
>>
>> where "StartCount" is set by the BSP at startup, and FinishedCount is
>> incremented by the APs themselves.
>>
> 
> I think FinishedCount used here is a typo. What the logic from the code 
> context here is want to get the failed Aps and return them. So it should use 
> RunningCount here, right? Also the FinishedCount may be bigger than 
> StartCount if many Aps has been disabled. Right? 

I agree FinishedCount looks questionable / out of place in the original
code.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.

2018-07-25 Thread Dong, Eric 
Hi Laszlo,


> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, July 25, 2018 7:47 PM
> To: Dong, Eric ; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu 
> Subject: Re: [edk2] [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount
> and volatile definition.
> 
> Hi Eric,
> 
> On 07/25/18 09:50, Eric Dong wrote:
> > The StartCount is duplicated with RunningCount, replace it with
> > RunningCount. Also the volatile for RunningCount is not needed.
> 
> after staring at this patch for a long time, I think it is correct.
> However, I suggest that we improve the commit message, because this patch
> actually does three things:
> 
> (1) It removes "volatile" from RunningCount.
> 
> [This is OK, because only the BSP modifies it.]
> 
> (2) [This is the tricky part.]
> 
> When we detect a timeout in CheckAllAPs(), and collect the list of failed 
> CPUs,
> the size of the list is derived from the following difference, before the 
> patch:
> 
>   StartCount - FinishedCount
> 
> where "StartCount" is set by the BSP at startup, and FinishedCount is
> incremented by the APs themselves.
> 

I think FinishedCount used here is a typo. What the logic from the code context 
here is want to get the failed Aps and return them. So it should use 
RunningCount here, right? Also the FinishedCount may be bigger than StartCount 
if many Aps has been disabled. Right? 

> The patch replaces this difference with
> 
>   StartCount - RunningCount
> 
> that is, the difference is no more calculated from the BSP's startup counter
> and the AP's shared finish counter, but from the RunningCount measurement
> that the BSP does itself, in CheckAllAPs().
> 
> [This change is OK to me as well, but we should be clear about it.]
> 
> (3) Finally, the patch changes the meaning of RunningCount. Before the patch,
> we have:
> 
> - StartCount: the number of APs the BSP stars up,
> - RunningCount: the number of finished APs that the BSP collected
> 
> After the patch, StartCount is removed, and RunningCount is *redefined* as
> the following difference:
> 
>   OLD_StartCount - OLD_RunningCount
> 
> Giving the number of APs that the BSP started up but hasn't collected yet.
> 
> [This change looks good to me as well.]
> 
> *
> 
> Importantly, what we see in the AllocatePool() argument, is the
> *composition* of steps (2) and (3).
> 
> If you agree, can you please update the commit message to include my points
> (1) through (3)? It's OK if you leave out my remarks in brackets [].

Agreed. Will use your content when commit the changes.

Thanks,
Eric

> 
> No need to repost just because of this, of course.
> 
> Thanks!
> Laszlo
> 
> >
> > Cc: Laszlo Ersek 
> > Cc: Ruiyu Ni 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong 
> > ---
> >  UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 +--
> > UefiCpuPkg/Library/MpInitLib/MpLib.h |  3 +--
> >  2 files changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index ff09a0e9e7..0e57cc86bf 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -1424,7 +1424,7 @@ CheckAllAPs (
> >  // value of state after setting the it to CpuStateIdle, so BSP can 
> > safely
> make use of its value.
> >  //
> >  if (GetApState(CpuData) == CpuStateIdle) {
> > -  CpuMpData->RunningCount ++;
> > +  CpuMpData->RunningCount --;
> >CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
> >
> >//
> > @@ -1449,7 +1449,7 @@ CheckAllAPs (
> >//
> >// If all APs finish, return EFI_SUCCESS.
> >//
> > -  if (CpuMpData->RunningCount == CpuMpData->StartCount) {
> > +  if (CpuMpData->RunningCount == 0) {
> >  return EFI_SUCCESS;
> >}
> >
> > @@ -1466,7 +1466,7 @@ CheckAllAPs (
> >  //
> >  if (CpuMpData->FailedCpuList != NULL) {
> >*CpuMpData->FailedCpuList =
> > - AllocatePool ((CpuMpData->StartCount - CpuMpData->FinishedCount
> + 1) * sizeof (UINTN));
> > + AllocatePool ((CpuMpData->RunningCount + 1) * sizeof
> > + (UINTN));
> >ASSERT (*CpuMpData->FailedCpuList != NULL);
> >  }
> >  ListIndex = 0;
> > @@ -2212,7 +2212,7 @@ StartupAllAPsWorker (
> >  return EFI_NOT_STARTED;
> >}
> >
> > -  CpuMpData->StartCount = 0;
> > +  CpuMpData->R

Re: [edk2] [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.

2018-07-25 Thread Laszlo Ersek 
Hi Eric,

On 07/25/18 09:50, Eric Dong wrote:
> The StartCount is duplicated with RunningCount, replace it with
> RunningCount. Also the volatile for RunningCount is not needed.

after staring at this patch for a long time, I think it is correct.
However, I suggest that we improve the commit message, because this
patch actually does three things:

(1) It removes "volatile" from RunningCount.

[This is OK, because only the BSP modifies it.]

(2) [This is the tricky part.]

When we detect a timeout in CheckAllAPs(), and collect the list of
failed CPUs, the size of the list is derived from the following
difference, before the patch:

  StartCount - FinishedCount

where "StartCount" is set by the BSP at startup, and FinishedCount is
incremented by the APs themselves.

The patch replaces this difference with

  StartCount - RunningCount

that is, the difference is no more calculated from the BSP's startup
counter and the AP's shared finish counter, but from the RunningCount
measurement that the BSP does itself, in CheckAllAPs().

[This change is OK to me as well, but we should be clear about it.]

(3) Finally, the patch changes the meaning of RunningCount. Before the
patch, we have:

- StartCount: the number of APs the BSP stars up,
- RunningCount: the number of finished APs that the BSP collected

After the patch, StartCount is removed, and RunningCount is *redefined*
as the following difference:

  OLD_StartCount - OLD_RunningCount

Giving the number of APs that the BSP started up but hasn't collected yet.

[This change looks good to me as well.]

*

Importantly, what we see in the AllocatePool() argument, is the
*composition* of steps (2) and (3).

If you agree, can you please update the commit message to include my
points (1) through (3)? It's OK if you leave out my remarks in brackets [].

No need to repost just because of this, of course.

Thanks!
Laszlo

> 
> Cc: Laszlo Ersek 
> Cc: Ruiyu Ni 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong 
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 +--
>  UefiCpuPkg/Library/MpInitLib/MpLib.h |  3 +--
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c 
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index ff09a0e9e7..0e57cc86bf 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1424,7 +1424,7 @@ CheckAllAPs (
>  // value of state after setting the it to CpuStateIdle, so BSP can 
> safely make use of its value.
>  //
>  if (GetApState(CpuData) == CpuStateIdle) {
> -  CpuMpData->RunningCount ++;
> +  CpuMpData->RunningCount --;
>CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
>  
>//
> @@ -1449,7 +1449,7 @@ CheckAllAPs (
>//
>// If all APs finish, return EFI_SUCCESS.
>//
> -  if (CpuMpData->RunningCount == CpuMpData->StartCount) {
> +  if (CpuMpData->RunningCount == 0) {
>  return EFI_SUCCESS;
>}
>  
> @@ -1466,7 +1466,7 @@ CheckAllAPs (
>  //
>  if (CpuMpData->FailedCpuList != NULL) {
>*CpuMpData->FailedCpuList =
> - AllocatePool ((CpuMpData->StartCount - CpuMpData->FinishedCount + 
> 1) * sizeof (UINTN));
> + AllocatePool ((CpuMpData->RunningCount + 1) * sizeof (UINTN));
>ASSERT (*CpuMpData->FailedCpuList != NULL);
>  }
>  ListIndex = 0;
> @@ -2212,7 +2212,7 @@ StartupAllAPsWorker (
>  return EFI_NOT_STARTED;
>}
>  
> -  CpuMpData->StartCount = 0;
> +  CpuMpData->RunningCount = 0;
>for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount; 
> ProcessorNumber++) {
>  CpuData = >CpuData[ProcessorNumber];
>  CpuData->Waiting = FALSE;
> @@ -,7 +,7 @@ StartupAllAPsWorker (
>  // Mark this processor as responsible for current calling.
>  //
>  CpuData->Waiting = TRUE;
> -CpuMpData->StartCount++;
> +CpuMpData->RunningCount++;
>}
>  }
>}
> @@ -2231,7 +2231,6 @@ StartupAllAPsWorker (
>CpuMpData->ProcArguments = ProcedureArgument;
>CpuMpData->SingleThread  = SingleThread;
>CpuMpData->FinishedCount = 0;
> -  CpuMpData->RunningCount  = 0;
>CpuMpData->FailedCpuList = FailedCpuList;
>CpuMpData->ExpectedTime  = CalculateTimeout (
> TimeoutInMicroseconds,
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h 
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 962bce685d..5002b7e9c0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -211,9 +211,8 @@ struct _CPU_MP_DATA {
>UINTN  BackupBuffer;
>UINTN  BackupBufferSize;
>  
> -  volatile UINT32StartCount;
>volatile UINT32FinishedCount;
> -  volatile UINT32RunningCount;
> +  UINT32 RunningCount;
>BOOLEANSingleThread;
>

[edk2] [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.

2018-07-25 Thread Eric Dong 
The StartCount is duplicated with RunningCount, replace it with
RunningCount. Also the volatile for RunningCount is not needed.

Cc: Laszlo Ersek 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong 
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 +--
 UefiCpuPkg/Library/MpInitLib/MpLib.h |  3 +--
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c 
b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index ff09a0e9e7..0e57cc86bf 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1424,7 +1424,7 @@ CheckAllAPs (
 // value of state after setting the it to CpuStateIdle, so BSP can safely 
make use of its value.
 //
 if (GetApState(CpuData) == CpuStateIdle) {
-  CpuMpData->RunningCount ++;
+  CpuMpData->RunningCount --;
   CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
 
   //
@@ -1449,7 +1449,7 @@ CheckAllAPs (
   //
   // If all APs finish, return EFI_SUCCESS.
   //
-  if (CpuMpData->RunningCount == CpuMpData->StartCount) {
+  if (CpuMpData->RunningCount == 0) {
 return EFI_SUCCESS;
   }
 
@@ -1466,7 +1466,7 @@ CheckAllAPs (
 //
 if (CpuMpData->FailedCpuList != NULL) {
   *CpuMpData->FailedCpuList =
- AllocatePool ((CpuMpData->StartCount - CpuMpData->FinishedCount + 1) 
* sizeof (UINTN));
+ AllocatePool ((CpuMpData->RunningCount + 1) * sizeof (UINTN));
   ASSERT (*CpuMpData->FailedCpuList != NULL);
 }
 ListIndex = 0;
@@ -2212,7 +2212,7 @@ StartupAllAPsWorker (
 return EFI_NOT_STARTED;
   }
 
-  CpuMpData->StartCount = 0;
+  CpuMpData->RunningCount = 0;
   for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount; 
ProcessorNumber++) {
 CpuData = >CpuData[ProcessorNumber];
 CpuData->Waiting = FALSE;
@@ -,7 +,7 @@ StartupAllAPsWorker (
 // Mark this processor as responsible for current calling.
 //
 CpuData->Waiting = TRUE;
-CpuMpData->StartCount++;
+CpuMpData->RunningCount++;
   }
 }
   }
@@ -2231,7 +2231,6 @@ StartupAllAPsWorker (
   CpuMpData->ProcArguments = ProcedureArgument;
   CpuMpData->SingleThread  = SingleThread;
   CpuMpData->FinishedCount = 0;
-  CpuMpData->RunningCount  = 0;
   CpuMpData->FailedCpuList = FailedCpuList;
   CpuMpData->ExpectedTime  = CalculateTimeout (
TimeoutInMicroseconds,
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h 
b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 962bce685d..5002b7e9c0 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -211,9 +211,8 @@ struct _CPU_MP_DATA {
   UINTN  BackupBuffer;
   UINTN  BackupBufferSize;
 
-  volatile UINT32StartCount;
   volatile UINT32FinishedCount;
-  volatile UINT32RunningCount;
+  UINT32 RunningCount;
   BOOLEANSingleThread;
   EFI_AP_PROCEDURE   Procedure;
   VOID   *ProcArguments;
-- 
2.15.0.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel