| Message ID | 20170907092010.3605-4-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
| Series | memory: Reduce memory use | expand |
On 09/07/2017 06:20 AM, Alexey Kardashevskiy wrote: > This allows sharing flat views between address spaces when the same root > memory region is used when creating a new address space. > > This adds a global list of flat views and a list of attached address > spaces per a flat view. Each address space references a flat view. > > This hard codes the dispatch tree building instead of doing so via > a memory listener. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > > This was suggested by Paolo in > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg05011.html > > I am not putting "Suggested-by" as I want to make sure that this is doing > what was actually suggested. > --- > include/exec/memory-internal.h | 6 +- > include/exec/memory.h | 9 +- > exec.c | 58 ++-------- > hw/pci/pci.c | 3 +- > memory.c | 253 +++++++++++++++++++++++++++-------------- > 5 files changed, 187 insertions(+), 142 deletions(-) > > diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h > index fb467acdba..8516e0b48f 100644 > --- a/include/exec/memory-internal.h > +++ b/include/exec/memory-internal.h > @@ -22,9 +22,11 @@ > #ifndef CONFIG_USER_ONLY > typedef struct AddressSpaceDispatch AddressSpaceDispatch; > > -void address_space_init_dispatch(AddressSpace *as); > void address_space_unregister(AddressSpace *as); > -void address_space_destroy_dispatch(AddressSpace *as); > +void address_space_dispatch_free(AddressSpaceDispatch *d); > +AddressSpaceDispatch *mem_begin(void); > +void mem_commit(AddressSpaceDispatch *d); > +void mem_add(AddressSpaceDispatch *d, MemoryRegionSection *section); > > extern const MemoryRegionOps unassigned_mem_ops; > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 83e82e90d5..41ab165302 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -27,6 +27,7 @@ > #include "qemu/rcu.h" > #include "hw/qdev-core.h" > > +typedef struct AddressSpaceDispatch AddressSpaceDispatch; > #define RAM_ADDR_INVALID (~(ram_addr_t)0) > > #define MAX_PHYS_ADDR_SPACE_BITS 62 > @@ -312,6 +313,7 @@ struct MemoryListener { > }; > > AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as); > +MemoryRegion *address_space_root(AddressSpace *as); > > /** > * AddressSpace: describes a mapping of addresses to #MemoryRegion objects > @@ -320,20 +322,17 @@ struct AddressSpace { > /* All fields are private. */ > struct rcu_head rcu; > char *name; > - MemoryRegion *root; > - int ref_count; > - bool malloced; > > /* Accessed via RCU. */ > struct FlatView *current_map; > > int ioeventfd_nb; > struct MemoryRegionIoeventfd *ioeventfds; > - struct AddressSpaceDispatch *dispatch; > - struct AddressSpaceDispatch *next_dispatch; > + > MemoryListener dispatch_listener; > QTAILQ_HEAD(memory_listeners_as, MemoryListener) listeners; > QTAILQ_ENTRY(AddressSpace) address_spaces_link; > + QTAILQ_ENTRY(AddressSpace) flat_view_link; > }; > > /** > diff --git a/exec.c b/exec.c > index 66f01f5fce..51243f57f4 100644 > --- a/exec.c > +++ b/exec.c > @@ -188,15 +188,12 @@ typedef struct PhysPageMap { > } PhysPageMap; > > struct AddressSpaceDispatch { > - struct rcu_head rcu; > - > MemoryRegionSection *mru_section; > /* This is a multi-level map on the physical address space. > * The bottom level has pointers to MemoryRegionSections. > */ > PhysPageEntry phys_map; > PhysPageMap map; > - AddressSpace *as; > }; > > typedef struct AddressSpaceDispatch AddressSpaceDispatch; > @@ -240,11 +237,6 @@ struct DirtyBitmapSnapshot { > unsigned long dirty[]; > }; > > -AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as) > -{ > - return atomic_rcu_read(&as->dispatch); > -} > - > #endif > > #if !defined(CONFIG_USER_ONLY) > @@ -1354,10 +1346,8 @@ static void register_multipage(AddressSpaceDispatch *d, > phys_page_set(d, start_addr >> TARGET_PAGE_BITS, num_pages, section_index); > } > > -static void mem_add(MemoryListener *listener, MemoryRegionSection *section) > +void mem_add(AddressSpaceDispatch *d, MemoryRegionSection *section) > { > - AddressSpace *as = container_of(listener, AddressSpace, dispatch_listener); > - AddressSpaceDispatch *d = as->next_dispatch; > MemoryRegionSection now = *section, remain = *section; > Int128 page_size = int128_make64(TARGET_PAGE_SIZE); > > @@ -2680,9 +2670,8 @@ static void io_mem_init(void) > NULL, UINT64_MAX); > } > > -static void mem_begin(MemoryListener *listener) > +AddressSpaceDispatch *mem_begin(void) > { > - AddressSpace *as = container_of(listener, AddressSpace, dispatch_listener); > AddressSpaceDispatch *d = g_new0(AddressSpaceDispatch, 1); > uint16_t n; > > @@ -2696,27 +2685,19 @@ static void mem_begin(MemoryListener *listener) > assert(n == PHYS_SECTION_WATCH); > > d->phys_map = (PhysPageEntry) { .ptr = PHYS_MAP_NODE_NIL, .skip = 1 }; > - as->next_dispatch = d; > + > + return d; > } > > -static void address_space_dispatch_free(AddressSpaceDispatch *d) > +void address_space_dispatch_free(AddressSpaceDispatch *d) > { > phys_sections_free(&d->map); > g_free(d); > } > > -static void mem_commit(MemoryListener *listener) > +void mem_commit(AddressSpaceDispatch *d) > { > - AddressSpace *as = container_of(listener, AddressSpace, dispatch_listener); > - AddressSpaceDispatch *cur = as->dispatch; > - AddressSpaceDispatch *next = as->next_dispatch; > - > - phys_page_compact_all(next, next->map.nodes_nb); > - > - atomic_rcu_set(&as->dispatch, next); > - if (cur) { > - call_rcu(cur, address_space_dispatch_free, rcu); > - } > + phys_page_compact_all(d, d->map.nodes_nb); > } > > static void tcg_commit(MemoryListener *listener) > @@ -2732,39 +2713,16 @@ static void tcg_commit(MemoryListener *listener) > * We reload the dispatch pointer now because cpu_reloading_memory_map() > * may have split the RCU critical section. > */ > - d = atomic_rcu_read(&cpuas->as->dispatch); > + d = address_space_to_dispatch(cpuas->as); > atomic_rcu_set(&cpuas->memory_dispatch, d); > tlb_flush(cpuas->cpu); > } > > -void address_space_init_dispatch(AddressSpace *as) > -{ > - as->dispatch = NULL; > - as->dispatch_listener = (MemoryListener) { > - .begin = mem_begin, > - .commit = mem_commit, > - .region_add = mem_add, > - .region_nop = mem_add, > - .priority = 0, > - }; > - memory_listener_register(&as->dispatch_listener, as); > -} > - > void address_space_unregister(AddressSpace *as) > { > memory_listener_unregister(&as->dispatch_listener); > } > > -void address_space_destroy_dispatch(AddressSpace *as) > -{ > - AddressSpaceDispatch *d = as->dispatch; > - > - atomic_rcu_set(&as->dispatch, NULL); > - if (d) { > - call_rcu(d, address_space_dispatch_free, rcu); > - } > -} > - > static void memory_map_init(void) > { > system_memory = g_malloc(sizeof(*system_memory)); > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 258fbe51e2..86b9e419fd 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -88,7 +88,8 @@ static void pci_init_bus_master(PCIDevice *pci_dev) > > memory_region_init_alias(&pci_dev->bus_master_enable_region, > OBJECT(pci_dev), "bus master", > - dma_as->root, 0, memory_region_size(dma_as->root)); > + address_space_root(dma_as), 0, > + memory_region_size(address_space_root(dma_as))); > memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); > memory_region_add_subregion(&pci_dev->bus_master_container_region, 0, > &pci_dev->bus_master_enable_region); > diff --git a/memory.c b/memory.c > index c6904a7deb..385a507511 100644 > --- a/memory.c > +++ b/memory.c > @@ -47,6 +47,9 @@ static QTAILQ_HEAD(memory_listeners, MemoryListener) memory_listeners > static QTAILQ_HEAD(, AddressSpace) address_spaces > = QTAILQ_HEAD_INITIALIZER(address_spaces); > > +static QTAILQ_HEAD(FlatViewList, FlatView) flat_views > + = QTAILQ_HEAD_INITIALIZER(flat_views); > + > typedef struct AddrRange AddrRange; > > /* > @@ -230,6 +233,11 @@ struct FlatView { > FlatRange *ranges; > unsigned nr; > unsigned nr_allocated; > + MemoryRegion *root; > + struct AddressSpaceDispatch *dispatch; > + > + QTAILQ_ENTRY(FlatView) flat_views_link; > + QTAILQ_HEAD(address_spaces, AddressSpace) address_spaces; > }; > > typedef struct AddressSpaceOps AddressSpaceOps; > @@ -259,12 +267,19 @@ static bool flatrange_equal(FlatRange *a, FlatRange *b) > && a->readonly == b->readonly; > } > > -static void flatview_init(FlatView *view) > +static void flatview_ref(FlatView *view); > + > +static FlatView *flatview_alloc(MemoryRegion *mr_root) > { > + FlatView *view; > + > + view = g_new0(FlatView, 1); > view->ref = 1; > - view->ranges = NULL; > - view->nr = 0; > - view->nr_allocated = 0; > + view->root = mr_root; > + memory_region_ref(mr_root); > + QTAILQ_INIT(&view->address_spaces); > + > + return view; > } > > /* Insert a range into a given position. Caller is responsible for maintaining > @@ -292,6 +307,10 @@ static void flatview_destroy(FlatView *view) > memory_region_unref(view->ranges[i].mr); > } > g_free(view->ranges); > + if (view->dispatch) { > + address_space_dispatch_free(view->dispatch); > + } > + memory_region_unref(view->root); > g_free(view); > } > > @@ -303,7 +322,7 @@ static void flatview_ref(FlatView *view) > static void flatview_unref(FlatView *view) > { > if (atomic_fetch_dec(&view->ref) == 1) { > - flatview_destroy(view); > + call_rcu(view, flatview_destroy, rcu); > } > } > > @@ -608,7 +627,7 @@ static AddressSpace *memory_region_to_address_space(MemoryRegion *mr) > mr = mr->container; > } > QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { > - if (mr == as->root) { > + if (mr == as->current_map->root) { > return as; > } > } > @@ -702,23 +721,6 @@ static void render_memory_region(FlatView *view, > } > } > > -/* Render a memory topology into a list of disjoint absolute ranges. */ > -static FlatView *generate_memory_topology(MemoryRegion *mr) > -{ > - FlatView *view; > - > - view = g_new(FlatView, 1); > - flatview_init(view); > - > - if (mr) { > - render_memory_region(view, mr, int128_zero(), > - addrrange_make(int128_zero(), int128_2_64()), false); > - } > - flatview_simplify(view); > - > - return view; > -} > - > static void address_space_add_del_ioeventfds(AddressSpace *as, > MemoryRegionIoeventfd *fds_new, > unsigned fds_new_nb, > @@ -769,12 +771,10 @@ static void address_space_add_del_ioeventfds(AddressSpace *as, > } > } > > -static FlatView *address_space_get_flatview(AddressSpace *as) > +static FlatView *flatview_get(FlatView *view) > { > - FlatView *view; > - > rcu_read_lock(); > - view = atomic_rcu_read(&as->current_map); > + view = atomic_rcu_read(&view); > flatview_ref(view); > rcu_read_unlock(); > return view; > @@ -789,7 +789,7 @@ static void address_space_update_ioeventfds(AddressSpace *as) > AddrRange tmp; > unsigned i; > > - view = address_space_get_flatview(as); > + view = flatview_get(as->current_map); > FOR_EACH_FLAT_RANGE(fr, view) { > for (i = 0; i < fr->mr->ioeventfd_nb; ++i) { > tmp = addrrange_shift(fr->mr->ioeventfds[i].addr, > @@ -881,28 +881,89 @@ static void address_space_update_topology_pass(AddressSpace *as, > } > } > > - > -static void address_space_update_topology(AddressSpace *as) > +static FlatView *address_space_update_flatview(FlatView *view) > { > - FlatView *old_view = address_space_get_flatview(as); > - FlatView *new_view = generate_memory_topology(as->root); > + AddressSpace *as, *asnext; > + FlatView *old_view = flatview_get(view); > + MemoryRegion *root = old_view->root; > + FlatView *new_view = flatview_alloc(root); > + unsigned iold, inew; > + FlatRange *frold, *frnew; > > - address_space_update_topology_pass(as, old_view, new_view, false); > - address_space_update_topology_pass(as, old_view, new_view, true); > + if (root) { > + render_memory_region(new_view, root, int128_zero(), > + addrrange_make(int128_zero(), int128_2_64()), > + false); > + flatview_simplify(new_view); > + } > > - /* Writes are protected by the BQL. */ > - atomic_rcu_set(&as->current_map, new_view); > - call_rcu(old_view, flatview_unref, rcu); > + new_view->dispatch = mem_begin(); > > - /* Note that all the old MemoryRegions are still alive up to this > - * point. This relieves most MemoryListeners from the need to > - * ref/unref the MemoryRegions they get---unless they use them > - * outside the iothread mutex, in which case precise reference > - * counting is necessary. > + /* > + * FIXME: this is cut-n-paste from address_space_update_topology_pass, > + * simplify it > */ > + iold = inew = 0; > + while (iold < old_view->nr || inew < new_view->nr) { > + if (iold < old_view->nr) { > + frold = &old_view->ranges[iold]; > + } else { > + frold = NULL; > + } > + if (inew < new_view->nr) { > + frnew = &new_view->ranges[inew]; > + } else { > + frnew = NULL; > + } > + > + if (frold > + && (!frnew > + || int128_lt(frold->addr.start, frnew->addr.start) > + || (int128_eq(frold->addr.start, frnew->addr.start) > + && !flatrange_equal(frold, frnew)))) { > + ++iold; > + } else if (frold && frnew && flatrange_equal(frold, frnew)) { > + /* In both and unchanged (except logging may have changed) */ > + MemoryRegionSection mrs = section_from_flat_range(frnew, > + new_view->dispatch); > + > + mem_add(new_view->dispatch, &mrs); > + > + ++iold; > + ++inew; > + } else { > + /* In new */ > + MemoryRegionSection mrs = section_from_flat_range(frnew, > + new_view->dispatch); > + > + mem_add(new_view->dispatch, &mrs); > + > + ++inew; > + } > + } > + > + mem_commit(new_view->dispatch); > + > + QTAILQ_FOREACH(as, &old_view->address_spaces, flat_view_link) { > + address_space_update_topology_pass(as, old_view, new_view, false); > + address_space_update_topology_pass(as, old_view, new_view, true); > + } > + > + QTAILQ_FOREACH_SAFE(as, &old_view->address_spaces, flat_view_link, asnext) { > + QTAILQ_REMOVE(&old_view->address_spaces, as, flat_view_link); > + flatview_unref(old_view); > + > + atomic_rcu_set(&as->current_map, new_view); > + > + flatview_ref(new_view); > + QTAILQ_INSERT_TAIL(&new_view->address_spaces, as, flat_view_link); > + > + address_space_update_ioeventfds(as); > + } > + > flatview_unref(old_view); > > - address_space_update_ioeventfds(as); > + return new_view; > } > > void memory_region_transaction_begin(void) > @@ -921,11 +982,31 @@ void memory_region_transaction_commit(void) > --memory_region_transaction_depth; > if (!memory_region_transaction_depth) { > if (memory_region_update_pending) { > + FlatView *view, *vnext; > + struct FlatViewList fwstmp = QTAILQ_HEAD_INITIALIZER(fwstmp); > + > MEMORY_LISTENER_CALL_GLOBAL(begin, Forward); > > - QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { > - address_space_update_topology(as); > + QTAILQ_FOREACH_SAFE(view, &flat_views, flat_views_link, vnext) { > + FlatView *old_view, *new_view; > + > + old_view = flatview_get(view); > + new_view = address_space_update_flatview(old_view); > + > + QTAILQ_REMOVE(&flat_views, old_view, flat_views_link); > + flatview_unref(old_view); > + flatview_unref(old_view); > + > + QTAILQ_INSERT_HEAD(&fwstmp, new_view, flat_views_link); > + > + flatview_unref(new_view); > } > + > + QTAILQ_FOREACH_SAFE(view, &fwstmp, flat_views_link, vnext) { > + QTAILQ_REMOVE(&fwstmp, view, flat_views_link); > + QTAILQ_INSERT_HEAD(&flat_views, view, flat_views_link); > + } > + > memory_region_update_pending = false; > MEMORY_LISTENER_CALL_GLOBAL(commit, Forward); > } else if (ioeventfd_update_pending) { > @@ -1834,7 +1915,7 @@ void memory_region_sync_dirty_bitmap(MemoryRegion *mr) > continue; > } > as = listener->address_space; > - view = address_space_get_flatview(as); > + view = flatview_get(as->current_map); > FOR_EACH_FLAT_RANGE(fr, view) { > if (fr->mr == mr) { > MemoryRegionSection mrs = section_from_flat_range(fr, > @@ -1938,7 +2019,7 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa > AddrRange tmp; > MemoryRegionSection section; > > - view = address_space_get_flatview(as); > + view = flatview_get(as->current_map); > FOR_EACH_FLAT_RANGE(fr, view) { > if (fr->mr == mr) { > section = (MemoryRegionSection) { > @@ -2350,7 +2431,7 @@ void memory_global_dirty_log_sync(void) > continue; > } > as = listener->address_space; > - view = address_space_get_flatview(as); > + view = flatview_get(as->current_map); > FOR_EACH_FLAT_RANGE(fr, view) { > if (fr->dirty_log_mask) { > MemoryRegionSection mrs = section_from_flat_range(fr, > @@ -2436,7 +2517,7 @@ static void listener_add_address_space(MemoryListener *listener, > } > } > > - view = address_space_get_flatview(as); > + view = flatview_get(as->current_map); > FOR_EACH_FLAT_RANGE(fr, view) { > MemoryRegionSection section = { > .mr = fr->mr, > @@ -2615,67 +2696,72 @@ void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset, > > void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name) > { > - memory_region_ref(root); > - memory_region_transaction_begin(); > - as->ref_count = 1; > - as->root = root; > - as->malloced = false; > - as->current_map = g_new(FlatView, 1); > - flatview_init(as->current_map); > + FlatView *view; > + > + as->current_map = NULL; > as->ioeventfd_nb = 0; > as->ioeventfds = NULL; > QTAILQ_INIT(&as->listeners); > QTAILQ_INSERT_TAIL(&address_spaces, as, address_spaces_link); > as->name = g_strdup(name ? name : "anonymous"); > - address_space_init_dispatch(as); > - memory_region_update_pending |= root->enabled; > - memory_region_transaction_commit(); > + > + QTAILQ_FOREACH(view, &flat_views, flat_views_link) { > + assert(root); > + if (view->root == root) { > + as->current_map = flatview_get(view); > + break; > + } > + } > + > + if (!as->current_map) { > + as->current_map = flatview_alloc(root); > + > + QTAILQ_INSERT_TAIL(&flat_views, as->current_map, flat_views_link); > + } > + > + QTAILQ_INSERT_TAIL(&as->current_map->address_spaces, as, flat_view_link); > +} > + > +MemoryRegion *address_space_root(AddressSpace *as) > +{ > + return as->current_map->root; > +} > + > +AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as) > +{ > + return atomic_rcu_read(&as->current_map)->dispatch; > } > > static void do_address_space_destroy(AddressSpace *as) > { > - bool do_free = as->malloced; > + FlatView *view = flatview_get(as->current_map); > > - address_space_destroy_dispatch(as); > assert(QTAILQ_EMPTY(&as->listeners)); > > - flatview_unref(as->current_map); > + QTAILQ_REMOVE(&view->address_spaces, as, flat_view_link); > + flatview_unref(view); > + > + flatview_unref(view); incorrect copy/paste? > + > g_free(as->name); > g_free(as->ioeventfds); > - memory_region_unref(as->root); > - if (do_free) { > - g_free(as); > - } > + g_free(as); > } > > 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) { > - as->ref_count++; > - return as; > - } > - } > - > as = g_malloc0(sizeof *as); > address_space_init(as, root, name); > - as->malloced = true; > + > return as; > } > > void address_space_destroy(AddressSpace *as) > { > - MemoryRegion *root = as->root; > - > - as->ref_count--; > - if (as->ref_count) { > - return; > - } > /* Flush out anything from MemoryListeners listening in on this */ > memory_region_transaction_begin(); > - as->root = NULL; > memory_region_transaction_commit(); > QTAILQ_REMOVE(&address_spaces, as, address_spaces_link); > address_space_unregister(as); > @@ -2684,7 +2770,6 @@ void address_space_destroy(AddressSpace *as) > * entries that the guest should never use. Wait for the old > * values to expire before freeing the data. > */ > - as->root = root; > call_rcu(as, do_address_space_destroy, rcu); > } > > @@ -2816,7 +2901,7 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f, > static void mtree_print_flatview(fprintf_function p, void *f, > AddressSpace *as) > { > - FlatView *view = address_space_get_flatview(as); > + FlatView *view = flatview_get(as->current_map); > FlatRange *range = &view->ranges[0]; > MemoryRegion *mr; > int n = view->nr; > @@ -2873,7 +2958,7 @@ void mtree_info(fprintf_function mon_printf, void *f, bool flatview) > > QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { > mon_printf(f, "address-space: %s\n", as->name); > - mtree_print_mr(mon_printf, f, as->root, 1, 0, &ml_head); > + mtree_print_mr(mon_printf, f, as->current_map->root, 1, 0, &ml_head); > mon_printf(f, "\n"); > } > >
On 08/09/17 06:53, Philippe Mathieu-Daudé wrote: > On 09/07/2017 06:20 AM, Alexey Kardashevskiy wrote: >> This allows sharing flat views between address spaces when the same root >> memory region is used when creating a new address space. >> >> This adds a global list of flat views and a list of attached address >> spaces per a flat view. Each address space references a flat view. >> >> This hard codes the dispatch tree building instead of doing so via >> a memory listener. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> >> This was suggested by Paolo in >> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg05011.html >> >> I am not putting "Suggested-by" as I want to make sure that this is doing >> what was actually suggested. >> --- >> include/exec/memory-internal.h | 6 +- >> include/exec/memory.h | 9 +- >> exec.c | 58 ++-------- >> hw/pci/pci.c | 3 +- >> memory.c | 253 >> +++++++++++++++++++++++++++-------------- >> 5 files changed, 187 insertions(+), 142 deletions(-) >> >> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h >> index fb467acdba..8516e0b48f 100644 >> --- a/include/exec/memory-internal.h >> +++ b/include/exec/memory-internal.h >> @@ -22,9 +22,11 @@ >> #ifndef CONFIG_USER_ONLY >> typedef struct AddressSpaceDispatch AddressSpaceDispatch; >> -void address_space_init_dispatch(AddressSpace *as); >> void address_space_unregister(AddressSpace *as); >> -void address_space_destroy_dispatch(AddressSpace *as); >> +void address_space_dispatch_free(AddressSpaceDispatch *d); >> +AddressSpaceDispatch *mem_begin(void); >> +void mem_commit(AddressSpaceDispatch *d); >> +void mem_add(AddressSpaceDispatch *d, MemoryRegionSection *section); >> extern const MemoryRegionOps unassigned_mem_ops; >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index 83e82e90d5..41ab165302 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -27,6 +27,7 @@ >> #include "qemu/rcu.h" >> #include "hw/qdev-core.h" >> +typedef struct AddressSpaceDispatch AddressSpaceDispatch; >> #define RAM_ADDR_INVALID (~(ram_addr_t)0) >> #define MAX_PHYS_ADDR_SPACE_BITS 62 >> @@ -312,6 +313,7 @@ struct MemoryListener { >> }; >> AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as); >> +MemoryRegion *address_space_root(AddressSpace *as); >> /** >> * AddressSpace: describes a mapping of addresses to #MemoryRegion objects >> @@ -320,20 +322,17 @@ struct AddressSpace { >> /* All fields are private. */ >> struct rcu_head rcu; >> char *name; >> - MemoryRegion *root; >> - int ref_count; >> - bool malloced; >> /* Accessed via RCU. */ >> struct FlatView *current_map; >> int ioeventfd_nb; >> struct MemoryRegionIoeventfd *ioeventfds; >> - struct AddressSpaceDispatch *dispatch; >> - struct AddressSpaceDispatch *next_dispatch; >> + >> MemoryListener dispatch_listener; >> QTAILQ_HEAD(memory_listeners_as, MemoryListener) listeners; >> QTAILQ_ENTRY(AddressSpace) address_spaces_link; >> + QTAILQ_ENTRY(AddressSpace) flat_view_link; >> }; >> /** >> diff --git a/exec.c b/exec.c >> index 66f01f5fce..51243f57f4 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -188,15 +188,12 @@ typedef struct PhysPageMap { >> } PhysPageMap; >> struct AddressSpaceDispatch { >> - struct rcu_head rcu; >> - >> MemoryRegionSection *mru_section; >> /* This is a multi-level map on the physical address space. >> * The bottom level has pointers to MemoryRegionSections. >> */ >> PhysPageEntry phys_map; >> PhysPageMap map; >> - AddressSpace *as; >> }; >> typedef struct AddressSpaceDispatch AddressSpaceDispatch; >> @@ -240,11 +237,6 @@ struct DirtyBitmapSnapshot { >> unsigned long dirty[]; >> }; >> -AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as) >> -{ >> - return atomic_rcu_read(&as->dispatch); >> -} >> - >> #endif >> #if !defined(CONFIG_USER_ONLY) >> @@ -1354,10 +1346,8 @@ static void >> register_multipage(AddressSpaceDispatch *d, >> phys_page_set(d, start_addr >> TARGET_PAGE_BITS, num_pages, >> section_index); >> } >> -static void mem_add(MemoryListener *listener, MemoryRegionSection >> *section) >> +void mem_add(AddressSpaceDispatch *d, MemoryRegionSection *section) >> { >> - AddressSpace *as = container_of(listener, AddressSpace, >> dispatch_listener); >> - AddressSpaceDispatch *d = as->next_dispatch; >> MemoryRegionSection now = *section, remain = *section; >> Int128 page_size = int128_make64(TARGET_PAGE_SIZE); >> @@ -2680,9 +2670,8 @@ static void io_mem_init(void) >> NULL, UINT64_MAX); >> } >> -static void mem_begin(MemoryListener *listener) >> +AddressSpaceDispatch *mem_begin(void) >> { >> - AddressSpace *as = container_of(listener, AddressSpace, >> dispatch_listener); >> AddressSpaceDispatch *d = g_new0(AddressSpaceDispatch, 1); >> uint16_t n; >> @@ -2696,27 +2685,19 @@ static void mem_begin(MemoryListener *listener) >> assert(n == PHYS_SECTION_WATCH); >> d->phys_map = (PhysPageEntry) { .ptr = PHYS_MAP_NODE_NIL, .skip >> = 1 }; >> - as->next_dispatch = d; >> + >> + return d; >> } >> -static void address_space_dispatch_free(AddressSpaceDispatch *d) >> +void address_space_dispatch_free(AddressSpaceDispatch *d) >> { >> phys_sections_free(&d->map); >> g_free(d); >> } >> -static void mem_commit(MemoryListener *listener) >> +void mem_commit(AddressSpaceDispatch *d) >> { >> - AddressSpace *as = container_of(listener, AddressSpace, >> dispatch_listener); >> - AddressSpaceDispatch *cur = as->dispatch; >> - AddressSpaceDispatch *next = as->next_dispatch; >> - >> - phys_page_compact_all(next, next->map.nodes_nb); >> - >> - atomic_rcu_set(&as->dispatch, next); >> - if (cur) { >> - call_rcu(cur, address_space_dispatch_free, rcu); >> - } >> + phys_page_compact_all(d, d->map.nodes_nb); >> } >> static void tcg_commit(MemoryListener *listener) >> @@ -2732,39 +2713,16 @@ static void tcg_commit(MemoryListener *listener) >> * We reload the dispatch pointer now because >> cpu_reloading_memory_map() >> * may have split the RCU critical section. >> */ >> - d = atomic_rcu_read(&cpuas->as->dispatch); >> + d = address_space_to_dispatch(cpuas->as); >> atomic_rcu_set(&cpuas->memory_dispatch, d); >> tlb_flush(cpuas->cpu); >> } >> -void address_space_init_dispatch(AddressSpace *as) >> -{ >> - as->dispatch = NULL; >> - as->dispatch_listener = (MemoryListener) { >> - .begin = mem_begin, >> - .commit = mem_commit, >> - .region_add = mem_add, >> - .region_nop = mem_add, >> - .priority = 0, >> - }; >> - memory_listener_register(&as->dispatch_listener, as); >> -} >> - >> void address_space_unregister(AddressSpace *as) >> { >> memory_listener_unregister(&as->dispatch_listener); >> } >> -void address_space_destroy_dispatch(AddressSpace *as) >> -{ >> - AddressSpaceDispatch *d = as->dispatch; >> - >> - atomic_rcu_set(&as->dispatch, NULL); >> - if (d) { >> - call_rcu(d, address_space_dispatch_free, rcu); >> - } >> -} >> - >> static void memory_map_init(void) >> { >> system_memory = g_malloc(sizeof(*system_memory)); >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index 258fbe51e2..86b9e419fd 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -88,7 +88,8 @@ static void pci_init_bus_master(PCIDevice *pci_dev) >> memory_region_init_alias(&pci_dev->bus_master_enable_region, >> OBJECT(pci_dev), "bus master", >> - dma_as->root, 0, >> memory_region_size(dma_as->root)); >> + address_space_root(dma_as), 0, >> + >> memory_region_size(address_space_root(dma_as))); >> memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); >> memory_region_add_subregion(&pci_dev->bus_master_container_region, 0, >> &pci_dev->bus_master_enable_region); >> diff --git a/memory.c b/memory.c >> index c6904a7deb..385a507511 100644 >> --- a/memory.c >> +++ b/memory.c >> @@ -47,6 +47,9 @@ static QTAILQ_HEAD(memory_listeners, MemoryListener) >> memory_listeners >> static QTAILQ_HEAD(, AddressSpace) address_spaces >> = QTAILQ_HEAD_INITIALIZER(address_spaces); >> +static QTAILQ_HEAD(FlatViewList, FlatView) flat_views >> + = QTAILQ_HEAD_INITIALIZER(flat_views); >> + >> typedef struct AddrRange AddrRange; >> /* >> @@ -230,6 +233,11 @@ struct FlatView { >> FlatRange *ranges; >> unsigned nr; >> unsigned nr_allocated; >> + MemoryRegion *root; >> + struct AddressSpaceDispatch *dispatch; >> + >> + QTAILQ_ENTRY(FlatView) flat_views_link; >> + QTAILQ_HEAD(address_spaces, AddressSpace) address_spaces; >> }; >> typedef struct AddressSpaceOps AddressSpaceOps; >> @@ -259,12 +267,19 @@ static bool flatrange_equal(FlatRange *a, FlatRange >> *b) >> && a->readonly == b->readonly; >> } >> -static void flatview_init(FlatView *view) >> +static void flatview_ref(FlatView *view); >> + >> +static FlatView *flatview_alloc(MemoryRegion *mr_root) >> { >> + FlatView *view; >> + >> + view = g_new0(FlatView, 1); >> view->ref = 1; >> - view->ranges = NULL; >> - view->nr = 0; >> - view->nr_allocated = 0; >> + view->root = mr_root; >> + memory_region_ref(mr_root); >> + QTAILQ_INIT(&view->address_spaces); >> + >> + return view; >> } >> /* Insert a range into a given position. Caller is responsible for >> maintaining >> @@ -292,6 +307,10 @@ static void flatview_destroy(FlatView *view) >> memory_region_unref(view->ranges[i].mr); >> } >> g_free(view->ranges); >> + if (view->dispatch) { >> + address_space_dispatch_free(view->dispatch); >> + } >> + memory_region_unref(view->root); >> g_free(view); >> } >> @@ -303,7 +322,7 @@ static void flatview_ref(FlatView *view) >> static void flatview_unref(FlatView *view) >> { >> if (atomic_fetch_dec(&view->ref) == 1) { >> - flatview_destroy(view); >> + call_rcu(view, flatview_destroy, rcu); >> } >> } >> @@ -608,7 +627,7 @@ static AddressSpace >> *memory_region_to_address_space(MemoryRegion *mr) >> mr = mr->container; >> } >> QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { >> - if (mr == as->root) { >> + if (mr == as->current_map->root) { >> return as; >> } >> } >> @@ -702,23 +721,6 @@ static void render_memory_region(FlatView *view, >> } >> } >> -/* Render a memory topology into a list of disjoint absolute ranges. */ >> -static FlatView *generate_memory_topology(MemoryRegion *mr) >> -{ >> - FlatView *view; >> - >> - view = g_new(FlatView, 1); >> - flatview_init(view); >> - >> - if (mr) { >> - render_memory_region(view, mr, int128_zero(), >> - addrrange_make(int128_zero(), >> int128_2_64()), false); >> - } >> - flatview_simplify(view); >> - >> - return view; >> -} >> - >> static void address_space_add_del_ioeventfds(AddressSpace *as, >> MemoryRegionIoeventfd >> *fds_new, >> unsigned fds_new_nb, >> @@ -769,12 +771,10 @@ static void >> address_space_add_del_ioeventfds(AddressSpace *as, >> } >> } >> -static FlatView *address_space_get_flatview(AddressSpace *as) >> +static FlatView *flatview_get(FlatView *view) >> { >> - FlatView *view; >> - >> rcu_read_lock(); >> - view = atomic_rcu_read(&as->current_map); >> + view = atomic_rcu_read(&view); >> flatview_ref(view); >> rcu_read_unlock(); >> return view; >> @@ -789,7 +789,7 @@ static void >> address_space_update_ioeventfds(AddressSpace *as) >> AddrRange tmp; >> unsigned i; >> - view = address_space_get_flatview(as); >> + view = flatview_get(as->current_map); >> FOR_EACH_FLAT_RANGE(fr, view) { >> for (i = 0; i < fr->mr->ioeventfd_nb; ++i) { >> tmp = addrrange_shift(fr->mr->ioeventfds[i].addr, >> @@ -881,28 +881,89 @@ static void >> address_space_update_topology_pass(AddressSpace *as, >> } >> } >> - >> -static void address_space_update_topology(AddressSpace *as) >> +static FlatView *address_space_update_flatview(FlatView *view) >> { >> - FlatView *old_view = address_space_get_flatview(as); >> - FlatView *new_view = generate_memory_topology(as->root); >> + AddressSpace *as, *asnext; >> + FlatView *old_view = flatview_get(view); >> + MemoryRegion *root = old_view->root; >> + FlatView *new_view = flatview_alloc(root); >> + unsigned iold, inew; >> + FlatRange *frold, *frnew; >> - address_space_update_topology_pass(as, old_view, new_view, false); >> - address_space_update_topology_pass(as, old_view, new_view, true); >> + if (root) { >> + render_memory_region(new_view, root, int128_zero(), >> + addrrange_make(int128_zero(), int128_2_64()), >> + false); >> + flatview_simplify(new_view); >> + } >> - /* Writes are protected by the BQL. */ >> - atomic_rcu_set(&as->current_map, new_view); >> - call_rcu(old_view, flatview_unref, rcu); >> + new_view->dispatch = mem_begin(); >> - /* Note that all the old MemoryRegions are still alive up to this >> - * point. This relieves most MemoryListeners from the need to >> - * ref/unref the MemoryRegions they get---unless they use them >> - * outside the iothread mutex, in which case precise reference >> - * counting is necessary. >> + /* >> + * FIXME: this is cut-n-paste from address_space_update_topology_pass, >> + * simplify it >> */ >> + iold = inew = 0; >> + while (iold < old_view->nr || inew < new_view->nr) { >> + if (iold < old_view->nr) { >> + frold = &old_view->ranges[iold]; >> + } else { >> + frold = NULL; >> + } >> + if (inew < new_view->nr) { >> + frnew = &new_view->ranges[inew]; >> + } else { >> + frnew = NULL; >> + } >> + >> + if (frold >> + && (!frnew >> + || int128_lt(frold->addr.start, frnew->addr.start) >> + || (int128_eq(frold->addr.start, frnew->addr.start) >> + && !flatrange_equal(frold, frnew)))) { >> + ++iold; >> + } else if (frold && frnew && flatrange_equal(frold, frnew)) { >> + /* In both and unchanged (except logging may have changed) */ >> + MemoryRegionSection mrs = section_from_flat_range(frnew, >> + new_view->dispatch); >> + >> + mem_add(new_view->dispatch, &mrs); >> + >> + ++iold; >> + ++inew; >> + } else { >> + /* In new */ >> + MemoryRegionSection mrs = section_from_flat_range(frnew, >> + new_view->dispatch); >> + >> + mem_add(new_view->dispatch, &mrs); >> + >> + ++inew; >> + } >> + } >> + >> + mem_commit(new_view->dispatch); >> + >> + QTAILQ_FOREACH(as, &old_view->address_spaces, flat_view_link) { >> + address_space_update_topology_pass(as, old_view, new_view, false); >> + address_space_update_topology_pass(as, old_view, new_view, true); >> + } >> + >> + QTAILQ_FOREACH_SAFE(as, &old_view->address_spaces, flat_view_link, >> asnext) { >> + QTAILQ_REMOVE(&old_view->address_spaces, as, flat_view_link); >> + flatview_unref(old_view); >> + >> + atomic_rcu_set(&as->current_map, new_view); >> + >> + flatview_ref(new_view); >> + QTAILQ_INSERT_TAIL(&new_view->address_spaces, as, flat_view_link); >> + >> + address_space_update_ioeventfds(as); >> + } >> + >> flatview_unref(old_view); >> - address_space_update_ioeventfds(as); >> + return new_view; >> } >> void memory_region_transaction_begin(void) >> @@ -921,11 +982,31 @@ void memory_region_transaction_commit(void) >> --memory_region_transaction_depth; >> if (!memory_region_transaction_depth) { >> if (memory_region_update_pending) { >> + FlatView *view, *vnext; >> + struct FlatViewList fwstmp = QTAILQ_HEAD_INITIALIZER(fwstmp); >> + >> MEMORY_LISTENER_CALL_GLOBAL(begin, Forward); >> - QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { >> - address_space_update_topology(as); >> + QTAILQ_FOREACH_SAFE(view, &flat_views, flat_views_link, >> vnext) { >> + FlatView *old_view, *new_view; >> + >> + old_view = flatview_get(view); >> + new_view = address_space_update_flatview(old_view); >> + >> + QTAILQ_REMOVE(&flat_views, old_view, flat_views_link); >> + flatview_unref(old_view); >> + flatview_unref(old_view); >> + >> + QTAILQ_INSERT_HEAD(&fwstmp, new_view, flat_views_link); >> + >> + flatview_unref(new_view); >> } >> + >> + QTAILQ_FOREACH_SAFE(view, &fwstmp, flat_views_link, vnext) { >> + QTAILQ_REMOVE(&fwstmp, view, flat_views_link); >> + QTAILQ_INSERT_HEAD(&flat_views, view, flat_views_link); >> + } >> + >> memory_region_update_pending = false; >> MEMORY_LISTENER_CALL_GLOBAL(commit, Forward); >> } else if (ioeventfd_update_pending) { >> @@ -1834,7 +1915,7 @@ void memory_region_sync_dirty_bitmap(MemoryRegion *mr) >> continue; >> } >> as = listener->address_space; >> - view = address_space_get_flatview(as); >> + view = flatview_get(as->current_map); >> FOR_EACH_FLAT_RANGE(fr, view) { >> if (fr->mr == mr) { >> MemoryRegionSection mrs = section_from_flat_range(fr, >> @@ -1938,7 +2019,7 @@ static void >> memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa >> AddrRange tmp; >> MemoryRegionSection section; >> - view = address_space_get_flatview(as); >> + view = flatview_get(as->current_map); >> FOR_EACH_FLAT_RANGE(fr, view) { >> if (fr->mr == mr) { >> section = (MemoryRegionSection) { >> @@ -2350,7 +2431,7 @@ void memory_global_dirty_log_sync(void) >> continue; >> } >> as = listener->address_space; >> - view = address_space_get_flatview(as); >> + view = flatview_get(as->current_map); >> FOR_EACH_FLAT_RANGE(fr, view) { >> if (fr->dirty_log_mask) { >> MemoryRegionSection mrs = section_from_flat_range(fr, >> @@ -2436,7 +2517,7 @@ static void >> listener_add_address_space(MemoryListener *listener, >> } >> } >> - view = address_space_get_flatview(as); >> + view = flatview_get(as->current_map); >> FOR_EACH_FLAT_RANGE(fr, view) { >> MemoryRegionSection section = { >> .mr = fr->mr, >> @@ -2615,67 +2696,72 @@ void >> memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset, >> void address_space_init(AddressSpace *as, MemoryRegion *root, const >> char *name) >> { >> - memory_region_ref(root); >> - memory_region_transaction_begin(); >> - as->ref_count = 1; >> - as->root = root; >> - as->malloced = false; >> - as->current_map = g_new(FlatView, 1); >> - flatview_init(as->current_map); >> + FlatView *view; >> + >> + as->current_map = NULL; >> as->ioeventfd_nb = 0; >> as->ioeventfds = NULL; >> QTAILQ_INIT(&as->listeners); >> QTAILQ_INSERT_TAIL(&address_spaces, as, address_spaces_link); >> as->name = g_strdup(name ? name : "anonymous"); >> - address_space_init_dispatch(as); >> - memory_region_update_pending |= root->enabled; >> - memory_region_transaction_commit(); >> + >> + QTAILQ_FOREACH(view, &flat_views, flat_views_link) { >> + assert(root); >> + if (view->root == root) { >> + as->current_map = flatview_get(view); >> + break; >> + } >> + } >> + >> + if (!as->current_map) { >> + as->current_map = flatview_alloc(root); >> + >> + QTAILQ_INSERT_TAIL(&flat_views, as->current_map, flat_views_link); >> + } >> + >> + QTAILQ_INSERT_TAIL(&as->current_map->address_spaces, as, >> flat_view_link); >> +} >> + >> +MemoryRegion *address_space_root(AddressSpace *as) >> +{ >> + return as->current_map->root; >> +} >> + >> +AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as) >> +{ >> + return atomic_rcu_read(&as->current_map)->dispatch; >> } >> static void do_address_space_destroy(AddressSpace *as) >> { >> - bool do_free = as->malloced; >> + FlatView *view = flatview_get(as->current_map); >> - address_space_destroy_dispatch(as); >> assert(QTAILQ_EMPTY(&as->listeners)); >> - flatview_unref(as->current_map); >> + QTAILQ_REMOVE(&view->address_spaces, as, flat_view_link); >> + flatview_unref(view); >> + >> + flatview_unref(view); This one removes a reference after unlinking an address space from the flatview. > > incorrect copy/paste? Nope, this one pairs flatview_get() from few lines above.
On 07/09/2017 11:20, Alexey Kardashevskiy wrote: > > /* Accessed via RCU. */ > struct FlatView *current_map; > > int ioeventfd_nb; > struct MemoryRegionIoeventfd *ioeventfds; > - struct AddressSpaceDispatch *dispatch; > - struct AddressSpaceDispatch *next_dispatch; > + The rough idea of the patch matches my suggestion indeed. However, I am not sure why all of the changes in patch 2 are needed. Once you have built the FlatView and the dispatch within it, you can still cache its dispatch tree in as->dispatch, and free it with RCU from flatview_destroy. This removes the need to use call_rcu from flatview_unref. In addition, you could change the computation of FlatView's root to resolve 2^64-sized aliases; also set it to NULL if the AddressSpace's root is disabled or the alias it resolves to is disabled (and so on recursively until a non-alias is found). This should remove the need for address_space_root() and the change to pci_init_bus_master. Paolo
On 11/09/17 17:40, Paolo Bonzini wrote: > On 07/09/2017 11:20, Alexey Kardashevskiy wrote: >> >> /* Accessed via RCU. */ >> struct FlatView *current_map; >> >> int ioeventfd_nb; >> struct MemoryRegionIoeventfd *ioeventfds; >> - struct AddressSpaceDispatch *dispatch; >> - struct AddressSpaceDispatch *next_dispatch; >> + > > The rough idea of the patch matches my suggestion indeed. However, I am > not sure why all of the changes in patch 2 are needed. For this: struct MemoryRegionSection { MemoryRegion *mr; - AddressSpace *address_space; + AddressSpaceDispatch *dispatch; as there are many ASes attached to the same flatview/dispatch. And because of that, there is also: struct IOMMUTLBEntry { - AddressSpace *target_as; + AddressSpaceDispatch *target_dispatch; as the "section" in address_space_get_iotlb_entry() does not have address_space any more, even though the only user of it - vhost_device_iotlb_miss() - only checks if (iotlb.target_dispatch != NULL). > Once you have built the FlatView and the dispatch within it, you can > still cache its dispatch tree in as->dispatch, and free it with RCU from > flatview_destroy. This removes the need to use call_rcu from > flatview_unref. Ok, I will do that. > In addition, you could change the computation of FlatView's root to > resolve 2^64-sized aliases; Here we reached the boundary of my english :) Roots are given when AS/Flatview is created, and aliases are resolved already. > also set it to NULL if the AddressSpace's > root is disabled or the alias it resolves to is disabled (and so on > recursively until a non-alias is found). This should remove the need > for address_space_root() and the change to pci_init_bus_master.
On 11/09/2017 11:06, Alexey Kardashevskiy wrote: > On 11/09/17 17:40, Paolo Bonzini wrote: >> On 07/09/2017 11:20, Alexey Kardashevskiy wrote: >>> >>> /* Accessed via RCU. */ >>> struct FlatView *current_map; >>> >>> int ioeventfd_nb; >>> struct MemoryRegionIoeventfd *ioeventfds; >>> - struct AddressSpaceDispatch *dispatch; >>> - struct AddressSpaceDispatch *next_dispatch; >>> + >> >> The rough idea of the patch matches my suggestion indeed. However, I am >> not sure why all of the changes in patch 2 are needed. > > For this: > > struct MemoryRegionSection { > MemoryRegion *mr; > - AddressSpace *address_space; > + AddressSpaceDispatch *dispatch; > > as there are many ASes attached to the same flatview/dispatch. Ok, this makes sense. Maybe it should be a flatview rather than an AddressSpaceDispatch (a FlatView is essentially a list of MemoryRegionSections; attaching the ASD to the FlatView is more or less an implementation detail). > And because of that, there is also: > > struct IOMMUTLBEntry { > - AddressSpace *target_as; > + AddressSpaceDispatch *target_dispatch; > > as the "section" in address_space_get_iotlb_entry() does not have > address_space any more, even though the only user of it - > vhost_device_iotlb_miss() - only checks if (iotlb.target_dispatch != NULL). You could change address_space_do_translate's "as" to AddressSpace **as. Then, address_space_get_iotlb_entry can populate the .target_as from the output value of the argument. >> In addition, you could change the computation of FlatView's root to >> resolve 2^64-sized aliases; > > Here we reached the boundary of my english :) > > Roots are given when AS/Flatview is created, and aliases are resolved already. > Currently, you're doing + if (view->root == root) { + as->current_map = flatview_get(view); + break; + } (and by the way the above doesn't resolve aliases; aliases are only resolved by render_memory_region). Instead, the FlatViews should be shared at transaction commit time. At the beginning of commit, the list of flat views is empty. As you process each AddressSpace, you resolve the root alias(*) and search for the resulting MemoryRegion in the list of FlatViews. If you find it, you do flatview_ref and point as->current_map to the FlatView you just found. Otherwise, you do generate_memory_topology etc. as in the old code. (*) something like MemoryRegion *mr = as->root; MemoryRegion *root_mr; while (mr->alias && !mr->alias_offset && int128_ge(mr->size, mr->alias->size)) { /* The alias is included in its entirety. Use it as * the "real" root, so that we can share more FlatViews. */ mr = mr->alias; } /* We found the "real" root, but maybe we can use a shared empty * FlatView instead? */ for (root_mr = mr; root_mr; root_mr = root_mr->alias) { if (!root_mr->enabled) { /* Use a shared empty FlatView. */ return NULL; } } return mr; Also: > +static FlatView *flatview_get(FlatView *view) > { > - FlatView *view; > - > rcu_read_lock(); > - view = atomic_rcu_read(&as->current_map); > + view = atomic_rcu_read(&view); This is "view = view" so it makes little sense, and then in the caller + view = flatview_get(as->current_map); as->current_map is accessed without atomic_rcu_read. So address_space_get_flatview must stay. Probably this will solve itself when you do the rest. Paolo >> also set it to NULL if the AddressSpace's >> root is disabled or the alias it resolves to is disabled (and so on >> recursively until a non-alias is found). This should remove the need >> for address_space_root() and the change to pci_init_bus_master. > > > >
On 11/09/17 19:37, Paolo Bonzini wrote: > On 11/09/2017 11:06, Alexey Kardashevskiy wrote: >> On 11/09/17 17:40, Paolo Bonzini wrote: >>> On 07/09/2017 11:20, Alexey Kardashevskiy wrote: >>>> >>>> /* Accessed via RCU. */ >>>> struct FlatView *current_map; >>>> >>>> int ioeventfd_nb; >>>> struct MemoryRegionIoeventfd *ioeventfds; >>>> - struct AddressSpaceDispatch *dispatch; >>>> - struct AddressSpaceDispatch *next_dispatch; >>>> + >>> >>> The rough idea of the patch matches my suggestion indeed. However, I am >>> not sure why all of the changes in patch 2 are needed. >> >> For this: >> >> struct MemoryRegionSection { >> MemoryRegion *mr; >> - AddressSpace *address_space; >> + AddressSpaceDispatch *dispatch; >> >> as there are many ASes attached to the same flatview/dispatch. > > Ok, this makes sense. Maybe it should be a flatview rather than an > AddressSpaceDispatch (a FlatView is essentially a list of > MemoryRegionSections; attaching the ASD to the FlatView is more or less > an implementation detail). The helpers I converted from AddressSpace to AddressSpaceDispatch do use dispatch structure only and do not use FlatView so it seemed logical. btw this address_space in MemoryRegionSection - it does not seem to make much sense in the PhysPageMap::sections array, it only makes sense when MemoryRegionSection uses as a temporary object when calling listeners. Will it make sense if we enforce MemoryRegionSection::address_space to be NULL in the array and not NULL when used temporary? > > >> And because of that, there is also: >> >> struct IOMMUTLBEntry { >> - AddressSpace *target_as; >> + AddressSpaceDispatch *target_dispatch; >> >> as the "section" in address_space_get_iotlb_entry() does not have >> address_space any more, even though the only user of it - >> vhost_device_iotlb_miss() - only checks if (iotlb.target_dispatch != NULL). > > You could change address_space_do_translate's "as" to AddressSpace **as. > Then, address_space_get_iotlb_entry can populate the .target_as from > the output value of the argument. Ok, since this seems better. > >>> In addition, you could change the computation of FlatView's root to >>> resolve 2^64-sized aliases; >> >> Here we reached the boundary of my english :) >> >> Roots are given when AS/Flatview is created, and aliases are resolved already. >> > > Currently, you're doing > > + if (view->root == root) { > + as->current_map = flatview_get(view); > + break; > + } > > (and by the way the above doesn't resolve aliases; aliases are only > resolved by render_memory_region). I am learning as we speak :) > > Instead, the FlatViews should be shared at transaction commit time. At > the beginning of commit, the list of flat views is empty. As you > process each AddressSpace, you resolve the root alias(*) and search for > the resulting MemoryRegion in the list of FlatViews. If you find it, > you do flatview_ref and point as->current_map to the FlatView you just > found. Otherwise, you do generate_memory_topology etc. as in the old code. > > (*) something like > > MemoryRegion *mr = as->root; > MemoryRegion *root_mr; > while (mr->alias && !mr->alias_offset && > int128_ge(mr->size, mr->alias->size)) { > /* The alias is included in its entirety. Use it as > * the "real" root, so that we can share more FlatViews. > */ > mr = mr->alias; > } > > /* We found the "real" root, but maybe we can use a shared empty > * FlatView instead? > */ > for (root_mr = mr; root_mr; root_mr = root_mr->alias) { > if (!root_mr->enabled) { > /* Use a shared empty FlatView. */ > return NULL; > } > } > > return mr; Ah, makes sense now, thanks. > > Also: > >> +static FlatView *flatview_get(FlatView *view) >> { >> - FlatView *view; >> - >> rcu_read_lock(); >> - view = atomic_rcu_read(&as->current_map); >> + view = atomic_rcu_read(&view); > > This is "view = view" so it makes little sense, and then in the caller > > + view = flatview_get(as->current_map); > > as->current_map is accessed without atomic_rcu_read. So > address_space_get_flatview must stay. Probably this will solve itself > when you do the rest. > > Paolo > >>> also set it to NULL if the AddressSpace's >>> root is disabled or the alias it resolves to is disabled (and so on >>> recursively until a non-alias is found). This should remove the need >>> for address_space_root() and the change to pci_init_bus_master. >> >> >> >> >
On 11/09/2017 14:08, Alexey Kardashevskiy wrote: >> Ok, this makes sense. Maybe it should be a flatview rather than an >> AddressSpaceDispatch (a FlatView is essentially a list of >> MemoryRegionSections; attaching the ASD to the FlatView is more or less >> an implementation detail). > The helpers I converted from AddressSpace to AddressSpaceDispatch do use > dispatch structure only and do not use FlatView so it seemed logical. Understood, but from a design POV FlatView makes more sense. > btw this address_space in MemoryRegionSection - it does not seem to make > much sense in the PhysPageMap::sections array, it only makes sense when > MemoryRegionSection uses as a temporary object when calling listeners. Will > it make sense if we enforce MemoryRegionSection::address_space to be NULL > in the array and not NULL when used temporary? memory_region_section_get_iotlb needs to access the AddressSpaceDispatch for sections stored in the PhysPageMap array, because memory_region_section_get_iotlb uses the ASD to compute the section index. Paolo
On 12/09/17 01:30, Paolo Bonzini wrote: > On 11/09/2017 14:08, Alexey Kardashevskiy wrote: >>> Ok, this makes sense. Maybe it should be a flatview rather than an >>> AddressSpaceDispatch (a FlatView is essentially a list of >>> MemoryRegionSections; attaching the ASD to the FlatView is more or less >>> an implementation detail). >> The helpers I converted from AddressSpace to AddressSpaceDispatch do use >> dispatch structure only and do not use FlatView so it seemed logical. > > Understood, but from a design POV FlatView makes more sense. > >> btw this address_space in MemoryRegionSection - it does not seem to make >> much sense in the PhysPageMap::sections array, it only makes sense when >> MemoryRegionSection uses as a temporary object when calling listeners. Will >> it make sense if we enforce MemoryRegionSection::address_space to be NULL >> in the array and not NULL when used temporary? > > memory_region_section_get_iotlb needs to access the AddressSpaceDispatch > for sections stored in the PhysPageMap array, because > memory_region_section_get_iotlb uses the ASD to compute the section index. Ohhh, not extremely trivial, out of curiosity - is that iotlb encoding described anywhere? Anyway, this can be simplified (or rather made more straightforward?) - tlb_set_page_with_attrs() can calculate the section index and pass it to memory_region_section_get_iotlb(). Still does not make much sense? It just looks quite useless to keep that address_space pointer alive just for one case which can easily avoid using this pointer.
On 12/09/2017 07:55, Alexey Kardashevskiy wrote: > On 12/09/17 01:30, Paolo Bonzini wrote: >> On 11/09/2017 14:08, Alexey Kardashevskiy wrote: >>>> Ok, this makes sense. Maybe it should be a flatview rather than an >>>> AddressSpaceDispatch (a FlatView is essentially a list of >>>> MemoryRegionSections; attaching the ASD to the FlatView is more or less >>>> an implementation detail). >>> The helpers I converted from AddressSpace to AddressSpaceDispatch do use >>> dispatch structure only and do not use FlatView so it seemed logical. >> >> Understood, but from a design POV FlatView makes more sense. >> >>> btw this address_space in MemoryRegionSection - it does not seem to make >>> much sense in the PhysPageMap::sections array, it only makes sense when >>> MemoryRegionSection uses as a temporary object when calling listeners. Will >>> it make sense if we enforce MemoryRegionSection::address_space to be NULL >>> in the array and not NULL when used temporary? >> >> memory_region_section_get_iotlb needs to access the AddressSpaceDispatch >> for sections stored in the PhysPageMap array, because >> memory_region_section_get_iotlb uses the ASD to compute the section index. > > Ohhh, not extremely trivial, out of curiosity - is that iotlb encoding > described anywhere? No, I don't think so. > Anyway, this can be simplified (or rather made more straightforward?) - > tlb_set_page_with_attrs() can calculate the section index and pass it to > memory_region_section_get_iotlb(). Still does not make much sense? It just > looks quite useless to keep that address_space pointer alive just for one > case which can easily avoid using this pointer. Hmm I suppose address_space_translate_for_iotlb knows the ASD and could also return the index, basically combining it and memory_region_section_get_iotlb() into one function. Paolo
On 12/09/17 17:12, Paolo Bonzini wrote: > On 12/09/2017 07:55, Alexey Kardashevskiy wrote: >> On 12/09/17 01:30, Paolo Bonzini wrote: >>> On 11/09/2017 14:08, Alexey Kardashevskiy wrote: >>>>> Ok, this makes sense. Maybe it should be a flatview rather than an >>>>> AddressSpaceDispatch (a FlatView is essentially a list of >>>>> MemoryRegionSections; attaching the ASD to the FlatView is more or less >>>>> an implementation detail). >>>> The helpers I converted from AddressSpace to AddressSpaceDispatch do use >>>> dispatch structure only and do not use FlatView so it seemed logical. >>> >>> Understood, but from a design POV FlatView makes more sense. >>> >>>> btw this address_space in MemoryRegionSection - it does not seem to make >>>> much sense in the PhysPageMap::sections array, it only makes sense when >>>> MemoryRegionSection uses as a temporary object when calling listeners. Will >>>> it make sense if we enforce MemoryRegionSection::address_space to be NULL >>>> in the array and not NULL when used temporary? >>> >>> memory_region_section_get_iotlb needs to access the AddressSpaceDispatch >>> for sections stored in the PhysPageMap array, because >>> memory_region_section_get_iotlb uses the ASD to compute the section index. >> >> Ohhh, not extremely trivial, out of curiosity - is that iotlb encoding >> described anywhere? > > No, I don't think so. > >> Anyway, this can be simplified (or rather made more straightforward?) - >> tlb_set_page_with_attrs() can calculate the section index and pass it to >> memory_region_section_get_iotlb(). Still does not make much sense? It just >> looks quite useless to keep that address_space pointer alive just for one >> case which can easily avoid using this pointer. > > Hmm I suppose address_space_translate_for_iotlb knows the ASD and could > also return the index, basically combining it and > memory_region_section_get_iotlb() into one function. Ok, good. One more question - how do we decide what goes to exec.c and what goes to memory.c? I have a temptation to simply embed AddressSpaceDispatch into FlatView instead of allocating it and storing the pointer (I'll probably avoid doing so for now anyway but still curios). The header comment for exec.c says it is "Virtual page mapping" but AddressSpaceDispatch is a physical address space map and it seems to fit into the memory.c's "Physical memory management" header comment.
diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h index fb467acdba..8516e0b48f 100644 --- a/include/exec/memory-internal.h +++ b/include/exec/memory-internal.h @@ -22,9 +22,11 @@ #ifndef CONFIG_USER_ONLY typedef struct AddressSpaceDispatch AddressSpaceDispatch; -void address_space_init_dispatch(AddressSpace *as); void address_space_unregister(AddressSpace *as); -void address_space_destroy_dispatch(AddressSpace *as); +void address_space_dispatch_free(AddressSpaceDispatch *d); +AddressSpaceDispatch *mem_begin(void); +void mem_commit(AddressSpaceDispatch *d); +void mem_add(AddressSpaceDispatch *d, MemoryRegionSection *section); extern const MemoryRegionOps unassigned_mem_ops; diff --git a/include/exec/memory.h b/include/exec/memory.h index 83e82e90d5..41ab165302 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -27,6 +27,7 @@ #include "qemu/rcu.h" #include "hw/qdev-core.h" +typedef struct AddressSpaceDispatch AddressSpaceDispatch; #define RAM_ADDR_INVALID (~(ram_addr_t)0) #define MAX_PHYS_ADDR_SPACE_BITS 62 @@ -312,6 +313,7 @@ struct MemoryListener { }; AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as); +MemoryRegion *address_space_root(AddressSpace *as); /** * AddressSpace: describes a mapping of addresses to #MemoryRegion objects @@ -320,20 +322,17 @@ struct AddressSpace { /* All fields are private. */ struct rcu_head rcu; char *name; - MemoryRegion *root; - int ref_count; - bool malloced; /* Accessed via RCU. */ struct FlatView *current_map; int ioeventfd_nb; struct MemoryRegionIoeventfd *ioeventfds; - struct AddressSpaceDispatch *dispatch; - struct AddressSpaceDispatch *next_dispatch; + MemoryListener dispatch_listener; QTAILQ_HEAD(memory_listeners_as, MemoryListener) listeners; QTAILQ_ENTRY(AddressSpace) address_spaces_link; + QTAILQ_ENTRY(AddressSpace) flat_view_link; }; /** diff --git a/exec.c b/exec.c index 66f01f5fce..51243f57f4 100644 --- a/exec.c +++ b/exec.c @@ -188,15 +188,12 @@ typedef struct PhysPageMap { } PhysPageMap; struct AddressSpaceDispatch { - struct rcu_head rcu; - MemoryRegionSection *mru_section; /* This is a multi-level map on the physical address space. * The bottom level has pointers to MemoryRegionSections. */ PhysPageEntry phys_map; PhysPageMap map; - AddressSpace *as; }; typedef struct AddressSpaceDispatch AddressSpaceDispatch; @@ -240,11 +237,6 @@ struct DirtyBitmapSnapshot { unsigned long dirty[]; }; -AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as) -{ - return atomic_rcu_read(&as->dispatch); -} - #endif #if !defined(CONFIG_USER_ONLY) @@ -1354,10 +1346,8 @@ static void register_multipage(AddressSpaceDispatch *d, phys_page_set(d, start_addr >> TARGET_PAGE_BITS, num_pages, section_index); } -static void mem_add(MemoryListener *listener, MemoryRegionSection *section) +void mem_add(AddressSpaceDispatch *d, MemoryRegionSection *section) { - AddressSpace *as = container_of(listener, AddressSpace, dispatch_listener); - AddressSpaceDispatch *d = as->next_dispatch; MemoryRegionSection now = *section, remain = *section; Int128 page_size = int128_make64(TARGET_PAGE_SIZE); @@ -2680,9 +2670,8 @@ static void io_mem_init(void) NULL, UINT64_MAX); } -static void mem_begin(MemoryListener *listener) +AddressSpaceDispatch *mem_begin(void) { - AddressSpace *as = container_of(listener, AddressSpace, dispatch_listener); AddressSpaceDispatch *d = g_new0(AddressSpaceDispatch, 1); uint16_t n; @@ -2696,27 +2685,19 @@ static void mem_begin(MemoryListener *listener) assert(n == PHYS_SECTION_WATCH); d->phys_map = (PhysPageEntry) { .ptr = PHYS_MAP_NODE_NIL, .skip = 1 }; - as->next_dispatch = d; + + return d; } -static void address_space_dispatch_free(AddressSpaceDispatch *d) +void address_space_dispatch_free(AddressSpaceDispatch *d) { phys_sections_free(&d->map); g_free(d); } -static void mem_commit(MemoryListener *listener) +void mem_commit(AddressSpaceDispatch *d) { - AddressSpace *as = container_of(listener, AddressSpace, dispatch_listener); - AddressSpaceDispatch *cur = as->dispatch; - AddressSpaceDispatch *next = as->next_dispatch; - - phys_page_compact_all(next, next->map.nodes_nb); - - atomic_rcu_set(&as->dispatch, next); - if (cur) { - call_rcu(cur, address_space_dispatch_free, rcu); - } + phys_page_compact_all(d, d->map.nodes_nb); } static void tcg_commit(MemoryListener *listener) @@ -2732,39 +2713,16 @@ static void tcg_commit(MemoryListener *listener) * We reload the dispatch pointer now because cpu_reloading_memory_map() * may have split the RCU critical section. */ - d = atomic_rcu_read(&cpuas->as->dispatch); + d = address_space_to_dispatch(cpuas->as); atomic_rcu_set(&cpuas->memory_dispatch, d); tlb_flush(cpuas->cpu); } -void address_space_init_dispatch(AddressSpace *as) -{ - as->dispatch = NULL; - as->dispatch_listener = (MemoryListener) { - .begin = mem_begin, - .commit = mem_commit, - .region_add = mem_add, - .region_nop = mem_add, - .priority = 0, - }; - memory_listener_register(&as->dispatch_listener, as); -} - void address_space_unregister(AddressSpace *as) { memory_listener_unregister(&as->dispatch_listener); } -void address_space_destroy_dispatch(AddressSpace *as) -{ - AddressSpaceDispatch *d = as->dispatch; - - atomic_rcu_set(&as->dispatch, NULL); - if (d) { - call_rcu(d, address_space_dispatch_free, rcu); - } -} - static void memory_map_init(void) { system_memory = g_malloc(sizeof(*system_memory)); diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 258fbe51e2..86b9e419fd 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -88,7 +88,8 @@ static void pci_init_bus_master(PCIDevice *pci_dev) memory_region_init_alias(&pci_dev->bus_master_enable_region, OBJECT(pci_dev), "bus master", - dma_as->root, 0, memory_region_size(dma_as->root)); + address_space_root(dma_as), 0, + memory_region_size(address_space_root(dma_as))); memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); memory_region_add_subregion(&pci_dev->bus_master_container_region, 0, &pci_dev->bus_master_enable_region); diff --git a/memory.c b/memory.c index c6904a7deb..385a507511 100644 --- a/memory.c +++ b/memory.c @@ -47,6 +47,9 @@ static QTAILQ_HEAD(memory_listeners, MemoryListener) memory_listeners static QTAILQ_HEAD(, AddressSpace) address_spaces = QTAILQ_HEAD_INITIALIZER(address_spaces); +static QTAILQ_HEAD(FlatViewList, FlatView) flat_views + = QTAILQ_HEAD_INITIALIZER(flat_views); + typedef struct AddrRange AddrRange; /* @@ -230,6 +233,11 @@ struct FlatView { FlatRange *ranges; unsigned nr; unsigned nr_allocated; + MemoryRegion *root; + struct AddressSpaceDispatch *dispatch; + + QTAILQ_ENTRY(FlatView) flat_views_link; + QTAILQ_HEAD(address_spaces, AddressSpace) address_spaces; }; typedef struct AddressSpaceOps AddressSpaceOps; @@ -259,12 +267,19 @@ static bool flatrange_equal(FlatRange *a, FlatRange *b) && a->readonly == b->readonly; } -static void flatview_init(FlatView *view) +static void flatview_ref(FlatView *view); + +static FlatView *flatview_alloc(MemoryRegion *mr_root) { + FlatView *view; + + view = g_new0(FlatView, 1); view->ref = 1; - view->ranges = NULL; - view->nr = 0; - view->nr_allocated = 0; + view->root = mr_root; + memory_region_ref(mr_root); + QTAILQ_INIT(&view->address_spaces); + + return view; } /* Insert a range into a given position. Caller is responsible for maintaining @@ -292,6 +307,10 @@ static void flatview_destroy(FlatView *view) memory_region_unref(view->ranges[i].mr); } g_free(view->ranges); + if (view->dispatch) { + address_space_dispatch_free(view->dispatch); + } + memory_region_unref(view->root); g_free(view); } @@ -303,7 +322,7 @@ static void flatview_ref(FlatView *view) static void flatview_unref(FlatView *view) { if (atomic_fetch_dec(&view->ref) == 1) { - flatview_destroy(view); + call_rcu(view, flatview_destroy, rcu); } } @@ -608,7 +627,7 @@ static AddressSpace *memory_region_to_address_space(MemoryRegion *mr) mr = mr->container; } QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { - if (mr == as->root) { + if (mr == as->current_map->root) { return as; } } @@ -702,23 +721,6 @@ static void render_memory_region(FlatView *view, } } -/* Render a memory topology into a list of disjoint absolute ranges. */ -static FlatView *generate_memory_topology(MemoryRegion *mr) -{ - FlatView *view; - - view = g_new(FlatView, 1); - flatview_init(view); - - if (mr) { - render_memory_region(view, mr, int128_zero(), - addrrange_make(int128_zero(), int128_2_64()), false); - } - flatview_simplify(view); - - return view; -} - static void address_space_add_del_ioeventfds(AddressSpace *as, MemoryRegionIoeventfd *fds_new, unsigned fds_new_nb, @@ -769,12 +771,10 @@ static void address_space_add_del_ioeventfds(AddressSpace *as, } } -static FlatView *address_space_get_flatview(AddressSpace *as) +static FlatView *flatview_get(FlatView *view) { - FlatView *view; - rcu_read_lock(); - view = atomic_rcu_read(&as->current_map); + view = atomic_rcu_read(&view); flatview_ref(view); rcu_read_unlock(); return view; @@ -789,7 +789,7 @@ static void address_space_update_ioeventfds(AddressSpace *as) AddrRange tmp; unsigned i; - view = address_space_get_flatview(as); + view = flatview_get(as->current_map); FOR_EACH_FLAT_RANGE(fr, view) { for (i = 0; i < fr->mr->ioeventfd_nb; ++i) { tmp = addrrange_shift(fr->mr->ioeventfds[i].addr, @@ -881,28 +881,89 @@ static void address_space_update_topology_pass(AddressSpace *as, } } - -static void address_space_update_topology(AddressSpace *as) +static FlatView *address_space_update_flatview(FlatView *view) { - FlatView *old_view = address_space_get_flatview(as); - FlatView *new_view = generate_memory_topology(as->root); + AddressSpace *as, *asnext; + FlatView *old_view = flatview_get(view); + MemoryRegion *root = old_view->root; + FlatView *new_view = flatview_alloc(root); + unsigned iold, inew; + FlatRange *frold, *frnew; - address_space_update_topology_pass(as, old_view, new_view, false); - address_space_update_topology_pass(as, old_view, new_view, true); + if (root) { + render_memory_region(new_view, root, int128_zero(), + addrrange_make(int128_zero(), int128_2_64()), + false); + flatview_simplify(new_view); + } - /* Writes are protected by the BQL. */ - atomic_rcu_set(&as->current_map, new_view); - call_rcu(old_view, flatview_unref, rcu); + new_view->dispatch = mem_begin(); - /* Note that all the old MemoryRegions are still alive up to this - * point. This relieves most MemoryListeners from the need to - * ref/unref the MemoryRegions they get---unless they use them - * outside the iothread mutex, in which case precise reference - * counting is necessary. + /* + * FIXME: this is cut-n-paste from address_space_update_topology_pass, + * simplify it */ + iold = inew = 0; + while (iold < old_view->nr || inew < new_view->nr) { + if (iold < old_view->nr) { + frold = &old_view->ranges[iold]; + } else { + frold = NULL; + } + if (inew < new_view->nr) { + frnew = &new_view->ranges[inew]; + } else { + frnew = NULL; + } + + if (frold + && (!frnew + || int128_lt(frold->addr.start, frnew->addr.start) + || (int128_eq(frold->addr.start, frnew->addr.start) + && !flatrange_equal(frold, frnew)))) { + ++iold; + } else if (frold && frnew && flatrange_equal(frold, frnew)) { + /* In both and unchanged (except logging may have changed) */ + MemoryRegionSection mrs = section_from_flat_range(frnew, + new_view->dispatch); + + mem_add(new_view->dispatch, &mrs); + + ++iold; + ++inew; + } else { + /* In new */ + MemoryRegionSection mrs = section_from_flat_range(frnew, + new_view->dispatch); + + mem_add(new_view->dispatch, &mrs); + + ++inew; + } + } + + mem_commit(new_view->dispatch); + + QTAILQ_FOREACH(as, &old_view->address_spaces, flat_view_link) { + address_space_update_topology_pass(as, old_view, new_view, false); + address_space_update_topology_pass(as, old_view, new_view, true); + } + + QTAILQ_FOREACH_SAFE(as, &old_view->address_spaces, flat_view_link, asnext) { + QTAILQ_REMOVE(&old_view->address_spaces, as, flat_view_link); + flatview_unref(old_view); + + atomic_rcu_set(&as->current_map, new_view); + + flatview_ref(new_view); + QTAILQ_INSERT_TAIL(&new_view->address_spaces, as, flat_view_link); + + address_space_update_ioeventfds(as); + } + flatview_unref(old_view); - address_space_update_ioeventfds(as); + return new_view; } void memory_region_transaction_begin(void) @@ -921,11 +982,31 @@ void memory_region_transaction_commit(void) --memory_region_transaction_depth; if (!memory_region_transaction_depth) { if (memory_region_update_pending) { + FlatView *view, *vnext; + struct FlatViewList fwstmp = QTAILQ_HEAD_INITIALIZER(fwstmp); + MEMORY_LISTENER_CALL_GLOBAL(begin, Forward); - QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { - address_space_update_topology(as); + QTAILQ_FOREACH_SAFE(view, &flat_views, flat_views_link, vnext) { + FlatView *old_view, *new_view; + + old_view = flatview_get(view); + new_view = address_space_update_flatview(old_view); + + QTAILQ_REMOVE(&flat_views, old_view, flat_views_link); + flatview_unref(old_view); + flatview_unref(old_view); + + QTAILQ_INSERT_HEAD(&fwstmp, new_view, flat_views_link); + + flatview_unref(new_view); } + + QTAILQ_FOREACH_SAFE(view, &fwstmp, flat_views_link, vnext) { + QTAILQ_REMOVE(&fwstmp, view, flat_views_link); + QTAILQ_INSERT_HEAD(&flat_views, view, flat_views_link); + } + memory_region_update_pending = false; MEMORY_LISTENER_CALL_GLOBAL(commit, Forward); } else if (ioeventfd_update_pending) { @@ -1834,7 +1915,7 @@ void memory_region_sync_dirty_bitmap(MemoryRegion *mr) continue; } as = listener->address_space; - view = address_space_get_flatview(as); + view = flatview_get(as->current_map); FOR_EACH_FLAT_RANGE(fr, view) { if (fr->mr == mr) { MemoryRegionSection mrs = section_from_flat_range(fr, @@ -1938,7 +2019,7 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa AddrRange tmp; MemoryRegionSection section; - view = address_space_get_flatview(as); + view = flatview_get(as->current_map); FOR_EACH_FLAT_RANGE(fr, view) { if (fr->mr == mr) { section = (MemoryRegionSection) { @@ -2350,7 +2431,7 @@ void memory_global_dirty_log_sync(void) continue; } as = listener->address_space; - view = address_space_get_flatview(as); + view = flatview_get(as->current_map); FOR_EACH_FLAT_RANGE(fr, view) { if (fr->dirty_log_mask) { MemoryRegionSection mrs = section_from_flat_range(fr, @@ -2436,7 +2517,7 @@ static void listener_add_address_space(MemoryListener *listener, } } - view = address_space_get_flatview(as); + view = flatview_get(as->current_map); FOR_EACH_FLAT_RANGE(fr, view) { MemoryRegionSection section = { .mr = fr->mr, @@ -2615,67 +2696,72 @@ void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset, void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name) { - memory_region_ref(root); - memory_region_transaction_begin(); - as->ref_count = 1; - as->root = root; - as->malloced = false; - as->current_map = g_new(FlatView, 1); - flatview_init(as->current_map); + FlatView *view; + + as->current_map = NULL; as->ioeventfd_nb = 0; as->ioeventfds = NULL; QTAILQ_INIT(&as->listeners); QTAILQ_INSERT_TAIL(&address_spaces, as, address_spaces_link); as->name = g_strdup(name ? name : "anonymous"); - address_space_init_dispatch(as); - memory_region_update_pending |= root->enabled; - memory_region_transaction_commit(); + + QTAILQ_FOREACH(view, &flat_views, flat_views_link) { + assert(root); + if (view->root == root) { + as->current_map = flatview_get(view); + break; + } + } + + if (!as->current_map) { + as->current_map = flatview_alloc(root); + + QTAILQ_INSERT_TAIL(&flat_views, as->current_map, flat_views_link); + } + + QTAILQ_INSERT_TAIL(&as->current_map->address_spaces, as, flat_view_link); +} + +MemoryRegion *address_space_root(AddressSpace *as) +{ + return as->current_map->root; +} + +AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as) +{ + return atomic_rcu_read(&as->current_map)->dispatch; } static void do_address_space_destroy(AddressSpace *as) { - bool do_free = as->malloced; + FlatView *view = flatview_get(as->current_map); - address_space_destroy_dispatch(as); assert(QTAILQ_EMPTY(&as->listeners)); - flatview_unref(as->current_map); + QTAILQ_REMOVE(&view->address_spaces, as, flat_view_link); + flatview_unref(view); + + flatview_unref(view); + g_free(as->name); g_free(as->ioeventfds); - memory_region_unref(as->root); - if (do_free) { - g_free(as); - } + g_free(as); } 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) { - as->ref_count++; - return as; - } - } - as = g_malloc0(sizeof *as); address_space_init(as, root, name); - as->malloced = true; + return as; } void address_space_destroy(AddressSpace *as) { - MemoryRegion *root = as->root; - - as->ref_count--; - if (as->ref_count) { - return; - } /* Flush out anything from MemoryListeners listening in on this */ memory_region_transaction_begin(); - as->root = NULL; memory_region_transaction_commit(); QTAILQ_REMOVE(&address_spaces, as, address_spaces_link); address_space_unregister(as); @@ -2684,7 +2770,6 @@ void address_space_destroy(AddressSpace *as) * entries that the guest should never use. Wait for the old * values to expire before freeing the data. */ - as->root = root; call_rcu(as, do_address_space_destroy, rcu); } @@ -2816,7 +2901,7 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f, static void mtree_print_flatview(fprintf_function p, void *f, AddressSpace *as) { - FlatView *view = address_space_get_flatview(as); + FlatView *view = flatview_get(as->current_map); FlatRange *range = &view->ranges[0]; MemoryRegion *mr; int n = view->nr; @@ -2873,7 +2958,7 @@ void mtree_info(fprintf_function mon_printf, void *f, bool flatview) QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { mon_printf(f, "address-space: %s\n", as->name); - mtree_print_mr(mon_printf, f, as->root, 1, 0, &ml_head); + mtree_print_mr(mon_printf, f, as->current_map->root, 1, 0, &ml_head); mon_printf(f, "\n"); }
This allows sharing flat views between address spaces when the same root memory region is used when creating a new address space. This adds a global list of flat views and a list of attached address spaces per a flat view. Each address space references a flat view. This hard codes the dispatch tree building instead of doing so via a memory listener. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- This was suggested by Paolo in https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg05011.html I am not putting "Suggested-by" as I want to make sure that this is doing what was actually suggested. --- include/exec/memory-internal.h | 6 +- include/exec/memory.h | 9 +- exec.c | 58 ++-------- hw/pci/pci.c | 3 +- memory.c | 253 +++++++++++++++++++++++++++-------------- 5 files changed, 187 insertions(+), 142 deletions(-)