| Message ID | 6eff688c-b62e-8796-f852-a855079f3175@redhat.com |
|---|---|
| State | New |
| Headers | show |
> >>> - 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
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
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;