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