public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: Eric Dong <eric.dong@intel.com>, edk2-devel@lists.01.org
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Subject: Re: [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.
Date: Wed, 25 Jul 2018 13:47:08 +0200	[thread overview]
Message-ID: <78bc8b2c-1579-5f6f-1d9c-eedc0899f6dc@redhat.com> (raw)
In-Reply-To: <20180725075020.240-3-eric.dong@intel.com>

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 <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 | 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 = &CpuMpData->CpuData[ProcessorNumber];
>      CpuData->Waiting = FALSE;
> @@ -2222,7 +2222,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 UINT32                StartCount;
>    volatile UINT32                FinishedCount;
> -  volatile UINT32                RunningCount;
> +  UINT32                         RunningCount;
>    BOOLEAN                        SingleThread;
>    EFI_AP_PROCEDURE               Procedure;
>    VOID                           *ProcArguments;
> 



  reply	other threads:[~2018-07-25 11:47 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-25  7:50 [Patch V3 0/3] StartAllAPs should not use disabled APs Eric Dong
2018-07-25  7:50 ` [Patch v3 1/3] UefiCpuPkg/MpInitLib: Remove redundant CpuStateFinished State Eric Dong
2018-07-25 10:54   ` Laszlo Ersek
2018-07-25 11:15     ` Dong, Eric
2018-07-26  5:18       ` Ni, Ruiyu
2018-07-25  7:50 ` [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition Eric Dong
2018-07-25 11:47   ` Laszlo Ersek [this message]
2018-07-25 12:09     ` Dong, Eric
2018-07-25 15:18       ` Laszlo Ersek
2018-07-26  5:22   ` Ni, Ruiyu
2018-07-25  7:50 ` [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs Eric Dong
2018-07-25 12:11   ` Laszlo Ersek
2018-07-25 12:44     ` Dong, Eric
2018-07-26  8:36   ` Ni, Ruiyu
2018-07-26  8:36     ` Ni, Ruiyu
2018-07-25 12:12 ` [Patch V3 0/3] StartAllAPs should not use disabled APs Laszlo Ersek
2018-07-25 15:18   ` Laszlo Ersek

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=78bc8b2c-1579-5f6f-1d9c-eedc0899f6dc@redhat.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