Hi Ray,
> -----Original Message-----
> From: Ni, Ray
> Sent: Monday, December 23, 2019 3:38 PM
> To: devel@edk2.groups.io; Dong, Eric <eric.d...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Subject: RE: [edk2-devel] [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm:
> Remove dependence between APs
>
> >
> > + WaitForSemaphore (&Token->RunningApCount);
> > +
> > + if (Token->RunningApCount == 0) {
> > + ReleaseSpinLock (Token->SpinLock);
> > }
>
> 1. if (InterlockedDecrement (&Token->RunningApCount) == 0) {
> ReleaseSpinLock (Token->SpinLock);
> }
>
> We should avoid checking RunningApCount directly because it's possible
> that AP#1 decrease the Count to 1 and before AP#1 checks the value against
> 0
> the Count is decreased by AP#2 to 0. So that causes AP#1 and AP#2 call
> ReleaseSpinLock() on the same SpinLock.
>
[[Eric]] good comments, will update it in next version.
> >
> > + // Decrease the count to mark this AP as finished.
>
> 2. BSP is also handled here. So this comment is mis-leading.
[[Eric]] will enhance the comments in next version.
>
> >
> > + //
> >
> > + if (Token != NULL) {
> > + WaitForSemaphore (&ProcToken->RunningApCount);
>
> 3. The code is written correctly but improperly IMO.
> Token is checked but ProcToken is deferenced.
> I suggest you check ProcToken directly.
[[Eric]] The other place in this function all check the Token status then
update code.
So this code consistent with other place. I will keep this code to keep
consistent.
Thanks,
Eric
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#52499): https://edk2.groups.io/g/devel/message/52499
Mute This Topic: https://groups.io/mt/69226060/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
