public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Zeng, Star" <star.zeng@intel.com>
To: Jian J Wang <jian.j.wang@intel.com>, edk2-devel@lists.01.org
Cc: Michael D Kinney <michael.d.kinney@intel.com>,
	Ruiyu Ni <ruiyu.ni@intel.com>, Jiewen Yao <jiewen.yao@intel.com>,
	Laszlo Ersek <lersek@redhat.com>,
	star.zeng@intel.com
Subject: Re: [PATCH v3 5/6] MdeModulePkg/Core: prevent re-acquire GCD memory lock
Date: Thu, 25 Oct 2018 11:22:58 +0800	[thread overview]
Message-ID: <a6181c15-3384-02c8-5957-7605b27ce12f@intel.com> (raw)
In-Reply-To: <20181024052620.4088-6-jian.j.wang@intel.com>

Some minor comments are added below.
With them addressed, Reviewed-by: Star Zeng <star.zeng@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.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@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;
>   }
>   
>   
> 



  reply	other threads:[~2018-10-25  3:23 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-24  5:26 [PATCH v3 0/6] Introduce freed-memory guard feature Jian J Wang
2018-10-24  5:26 ` [PATCH v3 1/6] MdeModulePkg: cleanup Heap Guard pool/page type PCD documentation Jian J Wang
2018-10-25  2:55   ` Zeng, Star
2018-10-25  4:21     ` Wang, Jian J
2018-10-24  5:26 ` [PATCH v3 2/6] MdeModulePkg: introduce UEFI freed-memory guard bit in HeapGuard PCD Jian J Wang
2018-10-25  3:02   ` Zeng, Star
2018-10-25  4:23     ` Wang, Jian J
2018-10-24  5:26 ` [PATCH v3 3/6] UefiCpuPkg/CpuDxe: consider freed-memory guard in non-stop mode Jian J Wang
2018-10-24  5:26 ` [PATCH v3 4/6] UefiCpuPkg/CpuDxe: prevent recursive calling of InitializePageTablePool Jian J Wang
2018-10-24  5:26 ` [PATCH v3 5/6] MdeModulePkg/Core: prevent re-acquire GCD memory lock Jian J Wang
2018-10-25  3:22   ` Zeng, Star [this message]
2018-10-25  4:24     ` Wang, Jian J
2018-10-24  5:26 ` [PATCH v3 6/6] MdeModulePkg/Core: add freed-memory guard feature Jian J Wang
2018-10-25  3:37   ` Zeng, Star
2018-10-25  4:29     ` Wang, Jian J
2018-10-25  6:28     ` Wang, Jian J

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=a6181c15-3384-02c8-5957-7605b27ce12f@intel.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