Sure. I'll change them. Thanks. Regards, Jian
> -----Original Message----- > From: Zeng, Star > Sent: Thursday, October 25, 2018 11:23 AM > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Ni, Ruiyu > <ruiyu...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Laszlo Ersek > <ler...@redhat.com>; Zeng, Star <star.z...@intel.com> > Subject: Re: [edk2] [PATCH v3 5/6] MdeModulePkg/Core: prevent re-acquire > GCD memory lock > > Some minor comments are added below. > With them addressed, Reviewed-by: Star Zeng <star.z...@intel.com>. > > On 2018/10/24 13:26, Jian J Wang wrote: > >> v3 changes: > >> a. drop the changes to CoreGetIoSpaceMap() because it won't cause > >> problem > >> b. refine the logic in changes of CoreGetMemorySpaceMap() > >> and add more comments > > > > This issue is hidden in current code but exposed by introduction > > of freed-memory guard feature due to the fact that the feature > > will turn all pool allocation to page allocation. > > > > The solution is move the memory allocation in CoreGetMemorySpaceMap() > > Use 'moving' instead of 'move'? > > > and CoreGetIoSpaceMap() to be out of the GCD memory map lock. > > Please remove CoreGetIoSpaceMap() as the code does not touch it at all. > > > > > Although it's rare, a while-loop is added to make sure that the count > > of descriptor of memory map is the same after memory allocation, because > > it's executed outside the GCD memory lock. > > > > CoreDumpGcdMemorySpaceMap() > > => CoreGetMemorySpaceMap() > > => CoreAcquireGcdMemoryLock () * > > AllocatePool() > > => InternalAllocatePool() > > => CoreAllocatePool() > > => CoreAllocatePoolI() > > => CoreAllocatePoolPagesI() > > => CoreAllocatePoolPages() > > => FindFreePages() > > => PromoteMemoryResource() > > => CoreAcquireGcdMemoryLock() ** > > > > Cc: Star Zeng <star.z...@intel.com> > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > Cc: Jiewen Yao <jiewen....@intel.com> > > Cc: Ruiyu Ni <ruiyu...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jian J Wang <jian.j.w...@intel.com> > > --- > > MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 80 +++++++++++++++++++++++++++- > ------------- > > 1 file changed, 54 insertions(+), 26 deletions(-) > > > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > > index d9c65a8045..bc02945721 100644 > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > > @@ -1691,10 +1691,10 @@ CoreGetMemorySpaceMap ( > > OUT EFI_GCD_MEMORY_SPACE_DESCRIPTOR **MemorySpaceMap > > ) > > { > > - EFI_STATUS Status; > > LIST_ENTRY *Link; > > EFI_GCD_MAP_ENTRY *Entry; > > EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Descriptor; > > + UINTN DescriptorCount; > > > > // > > // Make sure parameters are valid > > @@ -1706,38 +1706,66 @@ CoreGetMemorySpaceMap ( > > return EFI_INVALID_PARAMETER; > > } > > > > + *NumberOfDescriptors = 0; > > + *MemorySpaceMap = NULL; > > + > > CoreAcquireGcdMemoryLock (); > > Better to add comment for it like below quoted from the example at > https://lists.01.org/pipermail/edk2-devel/2018-October/031279.html given > by Laszlo. > > // > // Take the lock, for entering the loop with the lock held. > // > > > > > - // > > - // Count the number of descriptors > > - // > > - *NumberOfDescriptors = CoreCountGcdMapEntry > (&mGcdMemorySpaceMap); > > + while (TRUE) { > > + // > > + // Count the number of descriptors. It might be done more than once > because > > Use 'count' instead of 'number' to match the variable name? > > > + // there's code which has to be running outside the GCD lock. > > Use "AllocatePool() calling code below" instead of "there's code"? > > > + // > > + DescriptorCount = CoreCountGcdMapEntry (&mGcdMemorySpaceMap); > > + if (DescriptorCount == *NumberOfDescriptors) { > > + // > > + // Fill in the MemorySpaceMap if no memory space map change. > > + // > > + Descriptor = *MemorySpaceMap; > > + Link = mGcdMemorySpaceMap.ForwardLink; > > + while (Link != &mGcdMemorySpaceMap) { > > + Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, > EFI_GCD_MAP_SIGNATURE); > > + BuildMemoryDescriptor (Descriptor, Entry); > > + Descriptor++; > > + Link = Link->ForwardLink; > > + } > > > > - // > > - // Allocate the MemorySpaceMap > > - // > > - *MemorySpaceMap = AllocatePool (*NumberOfDescriptors * sizeof > (EFI_GCD_MEMORY_SPACE_DESCRIPTOR)); > > - if (*MemorySpaceMap == NULL) { > > - Status = EFI_OUT_OF_RESOURCES; > > - goto Done; > > - } > > + break; > > + } > > > > - // > > - // Fill in the MemorySpaceMap > > - // > > - Descriptor = *MemorySpaceMap; > > - Link = mGcdMemorySpaceMap.ForwardLink; > > - while (Link != &mGcdMemorySpaceMap) { > > - Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE); > > - BuildMemoryDescriptor (Descriptor, Entry); > > - Descriptor++; > > - Link = Link->ForwardLink; > > + // > > + // Release the lock before memory allocation, because it might cause > > + // GCD lock conflict in one of calling path in AllocatPool(). > > + // > > + CoreReleaseGcdMemoryLock (); > > + > > + // > > + // Allocate memory to store the MemorySpaceMap. Note it might be > already > > + // allocated if there's map descriptor change during memory allocation > > at > > + // last time. > > + // > > + if (*MemorySpaceMap != NULL) { > > + FreePool (*MemorySpaceMap); > > + } > > + > > + *MemorySpaceMap = AllocatePool (DescriptorCount * > > + sizeof > > (EFI_GCD_MEMORY_SPACE_DESCRIPTOR)); > > + if (*MemorySpaceMap == NULL) { > > + *NumberOfDescriptors = 0; > > + return EFI_OUT_OF_RESOURCES; > > + } > > + > > + // > > + // Save the descriptor number got before for another round of check to > make > > Use 'count' instead of 'number' to match the variable name? > > > + // sure we won't miss any, since we have code running outside the GCD > lock. > > + // > > + *NumberOfDescriptors = DescriptorCount; > > + CoreAcquireGcdMemoryLock (); > > Better to add comment for it like below quoted from the example at > https://lists.01.org/pipermail/edk2-devel/2018-October/031279.html given > by Laszlo. > > // > // Re-acquire the lock, for the next iteration. > // > > > } > > - Status = EFI_SUCCESS; > > > > -Done: > > CoreReleaseGcdMemoryLock (); > > Better to add comment for it like below quoted from the example at > https://lists.01.org/pipermail/edk2-devel/2018-October/031279.html given > by Laszlo. > > // > // We exited the loop with the lock held, release it. > // > > > Thanks, > Star > > > - return Status; > > + > > + return EFI_SUCCESS; > > } > > > > > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel