[Qemu-devel] [PATCH] Memory: use memory address space for cpu-memory

[Qemu-devel] [PATCH] Memory: use memory address space for cpu-memory

[Anthony Xu]

If cpu-memory address space is same as memory address space,
use memory address space for cpu-memory address space.

any memory region change causeaddress space to rebuild PhysPageMap,
rebuilding PhysPageMap is very expensive.

removing cpu-memory address space reduces the guest boot time and
memory usage.

Signed-off-by: Anthony Xu <anthony.xu@intel.com>
---
 cpus.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index 740b8dc..15c7a6a 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1748,8 +1748,13 @@ void qemu_init_vcpu(CPUState *cpu)
         /* If the target cpu hasn't set up any address spaces itself,
          * give it the default one.
          */
-        AddressSpace *as = address_space_init_shareable(cpu->memory,
-                                                        "cpu-memory");
+        AddressSpace *as;
+        if (cpu->memory == address_space_memory.root) {
+            address_space_memory.ref_count++;
+            as = &address_space_memory;
+        } else {
+            as = address_space_init_shareable(cpu->memory, "cpu-memory");
+        }
         cpu->num_ases = 1;
         cpu_address_space_init(cpu, as, 0);
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] Memory: use memory address space for cpu-memory

[Igor Mammedov]

On Tue, 16 May 2017 15:15:23 -0700
Anthony Xu <anthony.xu@intel.com> wrote:

> If cpu-memory address space is same as memory address space,
> use memory address space for cpu-memory address space.
> 
> any memory region change causeaddress space to rebuild PhysPageMap,
> rebuilding PhysPageMap is very expensive.
> 
> removing cpu-memory address space reduces the guest boot time and
> memory usage.
> 
> Signed-off-by: Anthony Xu <anthony.xu@intel.com>
> ---
>  cpus.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 740b8dc..15c7a6a 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1748,8 +1748,13 @@ void qemu_init_vcpu(CPUState *cpu)
>          /* If the target cpu hasn't set up any address spaces itself,
>           * give it the default one.
>           */
> -        AddressSpace *as = address_space_init_shareable(cpu->memory,
> -                                                        "cpu-memory");
> +        AddressSpace *as;
> +        if (cpu->memory == address_space_memory.root) {
> +            address_space_memory.ref_count++;
probably this would cause reference leak when vcpu is destroyed

> +            as = &address_space_memory;
> +        } else {
> +            as = address_space_init_shareable(cpu->memory, "cpu-memory");
> +        }
>          cpu->num_ases = 1;
>          cpu_address_space_init(cpu, as, 0);
>      }

Re: [Qemu-devel] [PATCH] Memory: use memory address space for cpu-memory

[Xu, Anthony]

> > If cpu-memory address space is same as memory address space,
> > use memory address space for cpu-memory address space.
> >
> > any memory region change causeaddress space to rebuild PhysPageMap,
> > rebuilding PhysPageMap is very expensive.
> >
> > removing cpu-memory address space reduces the guest boot time and
> > memory usage.
> >
> > Signed-off-by: Anthony Xu <anthony.xu@intel.com>
> > ---
> >  cpus.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/cpus.c b/cpus.c
> > index 740b8dc..15c7a6a 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -1748,8 +1748,13 @@ void qemu_init_vcpu(CPUState *cpu)
> >          /* If the target cpu hasn't set up any address spaces itself,
> >           * give it the default one.
> >           */
> > -        AddressSpace *as = address_space_init_shareable(cpu->memory,
> > -                                                        "cpu-memory");
> > +        AddressSpace *as;
> > +        if (cpu->memory == address_space_memory.root) {
> > +            address_space_memory.ref_count++;
> probably this would cause reference leak when vcpu is destroyed

I thought address_space_destroy is called when vcpu is unplugged,
seems that's not the case, then ref_count++ is not needed.

Any other comments?


Thanks,
Anthony

Re: [Qemu-devel] [PATCH] Memory: use memory address space for cpu-memory

[Paolo Bonzini]

On 17/05/2017 19:01, Xu, Anthony wrote:
>>> -        AddressSpace *as = address_space_init_shareable(cpu->memory,
>>> -                                                        "cpu-memory");
>>> +        AddressSpace *as;
>>> +        if (cpu->memory == address_space_memory.root) {
>>> +            address_space_memory.ref_count++;
>> probably this would cause reference leak when vcpu is destroyed
> I thought address_space_destroy is called when vcpu is unplugged,
> seems that's not the case, then ref_count++ is not needed.

It should be called.  Alternatively you could try adding a new function
to mark address_space_memory as a never-destroyed AddressSpace:

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 99e0f54d86..b27b288c8f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -290,6 +290,7 @@ struct AddressSpace {
     MemoryRegion *root;
     int ref_count;
     bool malloced;
+    bool shared;
 
     /* Accessed via RCU.  */
     struct FlatView *current_map;
diff --git a/memory.c b/memory.c
index b727f5ec0e..190cd3d5ce 100644
--- a/memory.c
+++ b/memory.c
@@ -2432,6 +2432,7 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
     as->ref_count = 1;
     as->root = root;
     as->malloced = false;
+    as->shared = false;
     as->current_map = g_new(FlatView, 1);
     flatview_init(as->current_map);
     as->ioeventfd_nb = 0;
@@ -2460,12 +2461,18 @@ static void do_address_space_destroy(AddressSpace *as)
     }
 }
 
+void address_space_init_static(AddressSpace *as, MemoryRegion *root, const char *name)
+{
+    address_space_init(as, root, name);
+    as->shared = true;
+}
+
 AddressSpace *address_space_init_shareable(MemoryRegion *root, const char *name)
 {
     AddressSpace *as;
 
     QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
-        if (root == as->root && as->malloced) {
+        if (root == as->root && as->shared) {
             as->ref_count++;
             return as;
         }
@@ -2474,6 +2481,7 @@ AddressSpace *address_space_init_shareable(MemoryRegion *root, const char *name)
     as = g_malloc0(sizeof *as);
     address_space_init(as, root, name);
     as->malloced = true;
+    as->shared = true;
     return as;
 }
 
@@ -2485,6 +2493,8 @@ void address_space_destroy(AddressSpace *as)
     if (as->ref_count) {
         return;
     }
+    assert(!as->shared || as->malloced);
+
     /* Flush out anything from MemoryListeners listening in on this */
     memory_region_transaction_begin();
     as->root = NULL;


then CPUs can keep using address_space_init_shareable.

Paolo

Re: [Qemu-devel] [PATCH] Memory: use memory address space for cpu-memory

[Xu, Anthony]

> >>> -        AddressSpace *as = address_space_init_shareable(cpu->memory,
> >>> -                                                        "cpu-memory");
> >>> +        AddressSpace *as;
> >>> +        if (cpu->memory == address_space_memory.root) {
> >>> +            address_space_memory.ref_count++;
> >> probably this would cause reference leak when vcpu is destroyed
> > I thought address_space_destroy is called when vcpu is unplugged,
> > seems that's not the case, then ref_count++ is not needed.
> 
> It should be called.  Alternatively you could try adding a new function
> to mark address_space_memory as a never-destroyed AddressSpace:
> 
This patch would do it, could you please submit this patch?

address_space_destroy is not called when vcpu is unplugged, that likely
causes memory leak. I will take a look when I have time.
If someone can take a look now, that'd be great.


Thanks,
Anthony



> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 99e0f54d86..b27b288c8f 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -290,6 +290,7 @@ struct AddressSpace {
>      MemoryRegion *root;
>      int ref_count;
>      bool malloced;
> +    bool shared;
> 
>      /* Accessed via RCU.  */
>      struct FlatView *current_map;
> diff --git a/memory.c b/memory.c
> index b727f5ec0e..190cd3d5ce 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2432,6 +2432,7 @@ void address_space_init(AddressSpace *as,
> MemoryRegion *root, const char *name)
>      as->ref_count = 1;
>      as->root = root;
>      as->malloced = false;
> +    as->shared = false;
>      as->current_map = g_new(FlatView, 1);
>      flatview_init(as->current_map);
>      as->ioeventfd_nb = 0;
> @@ -2460,12 +2461,18 @@ static void
> do_address_space_destroy(AddressSpace *as)
>      }
>  }
> 
> +void address_space_init_static(AddressSpace *as, MemoryRegion *root,
> const char *name)
> +{
> +    address_space_init(as, root, name);
> +    as->shared = true;
> +}
> +
>  AddressSpace *address_space_init_shareable(MemoryRegion *root, const
> char *name)
>  {
>      AddressSpace *as;
> 
>      QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> -        if (root == as->root && as->malloced) {
> +        if (root == as->root && as->shared) {
>              as->ref_count++;
>              return as;
>          }
> @@ -2474,6 +2481,7 @@ AddressSpace
> *address_space_init_shareable(MemoryRegion *root, const char *name)
>      as = g_malloc0(sizeof *as);
>      address_space_init(as, root, name);
>      as->malloced = true;
> +    as->shared = true;
>      return as;
>  }
> 
> @@ -2485,6 +2493,8 @@ void address_space_destroy(AddressSpace *as)
>      if (as->ref_count) {
>          return;
>      }
> +    assert(!as->shared || as->malloced);
> +
>      /* Flush out anything from MemoryListeners listening in on this */
>      memory_region_transaction_begin();
>      as->root = NULL;
> 
> 
> then CPUs can keep using address_space_init_shareable.
> 
> Paolo

Re: [Qemu-devel] [PATCH] Memory: use memory address space for cpu-memory

[Paolo Bonzini]

On 18/05/2017 23:48, Xu, Anthony wrote:
>> It should be called.  Alternatively you could try adding a new function
>> to mark address_space_memory as a never-destroyed AddressSpace:
>>
> This patch would do it, could you please submit this patch?

If you have tested it (together with the change in the initialization of
address_space_memory), I can do that.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] Memory: use memory address space for cpu-memory

[Xu, Anthony]

> On 18/05/2017 23:48, Xu, Anthony wrote:
> >> It should be called.  Alternatively you could try adding a new function
> >> to mark address_space_memory as a never-destroyed AddressSpace:
> >>
> > This patch would do it, could you please submit this patch?
> 
> If you have tested it (together with the change in the initialization of
> address_space_memory), I can do that.
> 

Based on your patch, I added the change in the initialization of 
address_space_memory. It works well in my setup, cpu-memory
address space doesn't show up as we expected.

Anthony

diff --git a/exec.c b/exec.c
index 96e3ac9..746dbbc 100644
--- a/exec.c
+++ b/exec.c
@@ -2712,7 +2712,7 @@ static void memory_map_init(void)
     system_memory = g_malloc(sizeof(*system_memory));

     memory_region_init(system_memory, NULL, "system", UINT64_MAX);
-    address_space_init(&address_space_memory, system_memory, "memory");
+    address_space_init_static(&address_space_memory, system_memory, "memory");

     system_io = g_malloc(sizeof(*system_io));
     memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
diff --git a/include/exec/memory.h b/include/exec/memory.h
index b27b288..6f44b79 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1395,6 +1395,17 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
 void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);

 /**
+ * address_space_init_static: initializes an static address space
+ *
+ * @as: an uninitialized #AddressSpace
+ * @root: a #MemoryRegion that routes addresses for the address space
+ * @name: an address space name.  The name is only used for debugging
+ *        output.
+ */
+void address_space_init_static(AddressSpace *as, MemoryRegion *root,
+       const char *name);
+
+/**
  * address_space_init_shareable: return an address space for a memory region,
  *                               creating it if it does not already exist
  *
diff --git a/memory.c b/memory.c
index 190cd3d..6c933d8 100644
--- a/memory.c
+++ b/memory.c
@@ -2461,7 +2461,8 @@ static void do_address_space_destroy(AddressSpace *as)
     }
 }

-void address_space_init_static(AddressSpace *as, MemoryRegion *root, const char *name)
+void address_space_init_static(AddressSpace *as, MemoryRegion *root,
+       const char *name)
 {
     address_space_init(as, root, name);
     as->shared = true;

Re: [Qemu-devel] [PATCH] Memory: use memory address space for cpu-memory

[Paolo Bonzini]

On 19/05/2017 01:28, Xu, Anthony wrote:
>> On 18/05/2017 23:48, Xu, Anthony wrote:
>>>> It should be called.  Alternatively you could try adding a new function
>>>> to mark address_space_memory as a never-destroyed AddressSpace:
>>>>
>>> This patch would do it, could you please submit this patch?
>>
>> If you have tested it (together with the change in the initialization of
>> address_space_memory), I can do that.
>>
> 
> Based on your patch, I added the change in the initialization of 
> address_space_memory. It works well in my setup, cpu-memory
> address space doesn't show up as we expected.

Thanks.

I am also thinking of resolving aliases early, and sharing the
AddressSpaceDispatch if the root memory region is the same.  This
hopefully will also reduce memory consumption and speed up AddressSpace
updates.

Paolo

> Anthony
> 
> diff --git a/exec.c b/exec.c
> index 96e3ac9..746dbbc 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2712,7 +2712,7 @@ static void memory_map_init(void)
>      system_memory = g_malloc(sizeof(*system_memory));
> 
>      memory_region_init(system_memory, NULL, "system", UINT64_MAX);
> -    address_space_init(&address_space_memory, system_memory, "memory");
> +    address_space_init_static(&address_space_memory, system_memory, "memory");
> 
>      system_io = g_malloc(sizeof(*system_io));
>      memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index b27b288..6f44b79 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1395,6 +1395,17 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
>  void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);
> 
>  /**
> + * address_space_init_static: initializes an static address space
> + *
> + * @as: an uninitialized #AddressSpace
> + * @root: a #MemoryRegion that routes addresses for the address space
> + * @name: an address space name.  The name is only used for debugging
> + *        output.
> + */
> +void address_space_init_static(AddressSpace *as, MemoryRegion *root,
> +       const char *name);
> +
> +/**
>   * address_space_init_shareable: return an address space for a memory region,
>   *                               creating it if it does not already exist
>   *
> diff --git a/memory.c b/memory.c
> index 190cd3d..6c933d8 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2461,7 +2461,8 @@ static void do_address_space_destroy(AddressSpace *as)
>      }
>  }
> 
> -void address_space_init_static(AddressSpace *as, MemoryRegion *root, const char *name)
> +void address_space_init_static(AddressSpace *as, MemoryRegion *root,
> +       const char *name)
>  {
>      address_space_init(as, root, name);
>      as->shared = true;
>