From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id EBEEB210C1ED8 for ; Wed, 25 Jul 2018 04:47:10 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 64B5A40704B0; Wed, 25 Jul 2018 11:47:09 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-225.rdu2.redhat.com [10.10.120.225]) by smtp.corp.redhat.com (Postfix) with ESMTP id B1E252026D68; Wed, 25 Jul 2018 11:47:08 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <20180725075020.240-1-eric.dong@intel.com> <20180725075020.240-3-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <78bc8b2c-1579-5f6f-1d9c-eedc0899f6dc@redhat.com> Date: Wed, 25 Jul 2018 13:47:08 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180725075020.240-3-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Wed, 25 Jul 2018 11:47:09 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Wed, 25 Jul 2018 11:47:09 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Jul 2018 11:47:11 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 = &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; >