Message ID | 1372444009-11544-4-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
Paolo Bonzini <pbonzini@redhat.com> writes: > With this change, a FlatView can be used even after a concurrent > update has replaced it. Because we do not have RCU, we use a > mutex to protect the small critical sections that read/write the > as->current_map pointer. Accesses to the FlatView can be done > outside the mutex. > > If a MemoryRegion will be used after the FlatView is unref-ed (or after > a MemoryListener callback is returned), a reference has to be added to > that MemoryRegion. For example, memory_region_find adds a reference to > the MemoryRegion that it returns. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > memory.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 69 insertions(+), 10 deletions(-) > > diff --git a/memory.c b/memory.c > index 319894e..bb92e17 100644 > --- a/memory.c > +++ b/memory.c > @@ -29,12 +29,26 @@ static unsigned memory_region_transaction_depth; > static bool memory_region_update_pending; > static bool global_dirty_log = false; > > +/* flat_view_mutex is taken around reading as->current_map; the critical > + * section is extremely short, so I'm using a single mutex for every AS. > + * We could also RCU for the read-side. > + * > + * The BQL is taken around transaction commits, hence both locks are taken > + * while writing to as->current_map (with the BQL taken outside). > + */ > +static QemuMutex flat_view_mutex; > + > static QTAILQ_HEAD(memory_listeners, MemoryListener) memory_listeners > = QTAILQ_HEAD_INITIALIZER(memory_listeners); > > static QTAILQ_HEAD(, AddressSpace) address_spaces > = QTAILQ_HEAD_INITIALIZER(address_spaces); > > +static void memory_init(void) > +{ > + qemu_mutex_init(&flat_view_mutex); > +} > + > typedef struct AddrRange AddrRange; > > /* > @@ -225,6 +239,7 @@ struct FlatRange { > * order. > */ > struct FlatView { > + unsigned ref; > FlatRange *ranges; > unsigned nr; > unsigned nr_allocated; > @@ -246,6 +261,7 @@ static bool flatrange_equal(FlatRange *a, FlatRange *b) > > static void flatview_init(FlatView *view) > { > + view->ref = 1; > view->ranges = NULL; > view->nr = 0; > view->nr_allocated = 0; > @@ -279,6 +295,18 @@ static void flatview_destroy(FlatView *view) > g_free(view); > } > > +static void flatview_ref(FlatView *view) > +{ > + __sync_fetch_and_add(&view->ref, 1); > +} > + > +static void flatview_unref(FlatView *view) > +{ > + if (__sync_fetch_and_sub(&view->ref, 1) == 1) { > + flatview_destroy(view); > + } > +} > + > static bool can_merge(FlatRange *r1, FlatRange *r2) > { > return int128_eq(addrrange_end(r1->addr), r2->addr.start) > @@ -578,6 +606,17 @@ static void address_space_add_del_ioeventfds(AddressSpace *as, > } > } > > +static FlatView *address_space_get_flatview(AddressSpace *as) > +{ > + FlatView *view; > + > + qemu_mutex_lock(&flat_view_mutex); > + view = as->current_map; > + flatview_ref(view); > + qemu_mutex_unlock(&flat_view_mutex); > + return view; > +} > + > static void address_space_update_ioeventfds(AddressSpace *as) > { > FlatView *view; > @@ -587,7 +626,7 @@ static void address_space_update_ioeventfds(AddressSpace *as) > AddrRange tmp; > unsigned i; > > - view = as->current_map; > + view = address_space_get_flatview(as); > FOR_EACH_FLAT_RANGE(fr, view) { > for (i = 0; i < fr->mr->ioeventfd_nb; ++i) { > tmp = addrrange_shift(fr->mr->ioeventfds[i].addr, > @@ -609,6 +648,7 @@ static void address_space_update_ioeventfds(AddressSpace *as) > g_free(as->ioeventfds); > as->ioeventfds = ioeventfds; > as->ioeventfd_nb = ioeventfd_nb; > + flatview_unref(view); > } > > static void address_space_update_topology_pass(AddressSpace *as, > @@ -676,14 +716,25 @@ static void address_space_update_topology_pass(AddressSpace *as, > > static void address_space_update_topology(AddressSpace *as) > { > - FlatView *old_view = as->current_map; > + FlatView *old_view = address_space_get_flatview(as); > FlatView *new_view = generate_memory_topology(as->root); > > address_space_update_topology_pass(as, old_view, new_view, false); > address_space_update_topology_pass(as, old_view, new_view, true); > > + qemu_mutex_lock(&flat_view_mutex); > + flatview_unref(as->current_map); > as->current_map = new_view; > - flatview_destroy(old_view); > + qemu_mutex_unlock(&flat_view_mutex); > + > + /* 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. > + */ > + flatview_unref(old_view); > + > address_space_update_ioeventfds(as); > } > > @@ -1146,12 +1197,13 @@ void memory_region_sync_dirty_bitmap(MemoryRegion *mr) > FlatRange *fr; > > QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { > - FlatView *view = as->current_map; > + FlatView *view = address_space_get_flatview(as); > FOR_EACH_FLAT_RANGE(fr, view) { > if (fr->mr == mr) { > MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, log_sync); > } > } > + flatview_unref(view); > } > } > > @@ -1203,7 +1255,7 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa > AddrRange tmp; > MemoryRegionSection section; > > - view = as->current_map; > + view = address_space_get_flatview(as); > FOR_EACH_FLAT_RANGE(fr, view) { > if (fr->mr == mr) { > section = (MemoryRegionSection) { > @@ -1229,6 +1281,7 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa > } > } > } > + flatview_unref(view); > } > > static void memory_region_update_coalesced_range(MemoryRegion *mr) > @@ -1520,7 +1573,7 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr, > as = memory_region_to_address_space(root); > range = addrrange_make(int128_make64(addr), int128_make64(size)); > > - view = as->current_map; > + view = address_space_get_flatview(as); > fr = flatview_lookup(view, range); > if (!fr) { > return ret; > @@ -1541,6 +1594,7 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr, > ret.readonly = fr->readonly; > memory_region_ref(ret.mr); > > + flatview_unref(view); > return ret; > } > > @@ -1549,10 +1603,11 @@ void address_space_sync_dirty_bitmap(AddressSpace *as) > FlatView *view; > FlatRange *fr; > > - view = as->current_map; > + view = address_space_get_flatview(as); > FOR_EACH_FLAT_RANGE(fr, view) { > MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, log_sync); > } > + flatview_unref(view); > } > > void memory_global_dirty_log_start(void) > @@ -1584,7 +1639,7 @@ static void listener_add_address_space(MemoryListener *listener, > } > } > > - view = as->current_map; > + view = address_space_get_flatview(as); > FOR_EACH_FLAT_RANGE(fr, view) { > MemoryRegionSection section = { > .mr = fr->mr, > @@ -1598,6 +1653,7 @@ static void listener_add_address_space(MemoryListener *listener, > listener->region_add(listener, §ion); > } > } > + flatview_unref(view); > } > > void memory_listener_register(MemoryListener *listener, AddressSpace *filter) > @@ -1631,6 +1687,10 @@ void memory_listener_unregister(MemoryListener *listener) > > void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name) > { > + if (QTAILQ_EMPTY(&address_spaces)) { > + memory_init(); > + } > + While this is fine, I see no harm in using a type_init() constructor to do the global initialization. Weirdness could ensue if we ever supported removing address spaces which isn't that crazy of an idea I think. Regards, Anthony Liguori > memory_region_transaction_begin(); > as->root = root; > as->current_map = g_new(FlatView, 1); > @@ -1652,9 +1712,8 @@ void address_space_destroy(AddressSpace *as) > memory_region_transaction_commit(); > QTAILQ_REMOVE(&address_spaces, as, address_spaces_link); > address_space_destroy_dispatch(as); > - flatview_destroy(as->current_map); > + flatview_unref(as->current_map); > g_free(as->name); > - g_free(as->current_map); > g_free(as->ioeventfds); > } > > -- > 1.8.1.4
diff --git a/memory.c b/memory.c index 319894e..bb92e17 100644 --- a/memory.c +++ b/memory.c @@ -29,12 +29,26 @@ static unsigned memory_region_transaction_depth; static bool memory_region_update_pending; static bool global_dirty_log = false; +/* flat_view_mutex is taken around reading as->current_map; the critical + * section is extremely short, so I'm using a single mutex for every AS. + * We could also RCU for the read-side. + * + * The BQL is taken around transaction commits, hence both locks are taken + * while writing to as->current_map (with the BQL taken outside). + */ +static QemuMutex flat_view_mutex; + static QTAILQ_HEAD(memory_listeners, MemoryListener) memory_listeners = QTAILQ_HEAD_INITIALIZER(memory_listeners); static QTAILQ_HEAD(, AddressSpace) address_spaces = QTAILQ_HEAD_INITIALIZER(address_spaces); +static void memory_init(void) +{ + qemu_mutex_init(&flat_view_mutex); +} + typedef struct AddrRange AddrRange; /* @@ -225,6 +239,7 @@ struct FlatRange { * order. */ struct FlatView { + unsigned ref; FlatRange *ranges; unsigned nr; unsigned nr_allocated; @@ -246,6 +261,7 @@ static bool flatrange_equal(FlatRange *a, FlatRange *b) static void flatview_init(FlatView *view) { + view->ref = 1; view->ranges = NULL; view->nr = 0; view->nr_allocated = 0; @@ -279,6 +295,18 @@ static void flatview_destroy(FlatView *view) g_free(view); } +static void flatview_ref(FlatView *view) +{ + __sync_fetch_and_add(&view->ref, 1); +} + +static void flatview_unref(FlatView *view) +{ + if (__sync_fetch_and_sub(&view->ref, 1) == 1) { + flatview_destroy(view); + } +} + static bool can_merge(FlatRange *r1, FlatRange *r2) { return int128_eq(addrrange_end(r1->addr), r2->addr.start) @@ -578,6 +606,17 @@ static void address_space_add_del_ioeventfds(AddressSpace *as, } } +static FlatView *address_space_get_flatview(AddressSpace *as) +{ + FlatView *view; + + qemu_mutex_lock(&flat_view_mutex); + view = as->current_map; + flatview_ref(view); + qemu_mutex_unlock(&flat_view_mutex); + return view; +} + static void address_space_update_ioeventfds(AddressSpace *as) { FlatView *view; @@ -587,7 +626,7 @@ static void address_space_update_ioeventfds(AddressSpace *as) AddrRange tmp; unsigned i; - view = as->current_map; + view = address_space_get_flatview(as); FOR_EACH_FLAT_RANGE(fr, view) { for (i = 0; i < fr->mr->ioeventfd_nb; ++i) { tmp = addrrange_shift(fr->mr->ioeventfds[i].addr, @@ -609,6 +648,7 @@ static void address_space_update_ioeventfds(AddressSpace *as) g_free(as->ioeventfds); as->ioeventfds = ioeventfds; as->ioeventfd_nb = ioeventfd_nb; + flatview_unref(view); } static void address_space_update_topology_pass(AddressSpace *as, @@ -676,14 +716,25 @@ static void address_space_update_topology_pass(AddressSpace *as, static void address_space_update_topology(AddressSpace *as) { - FlatView *old_view = as->current_map; + FlatView *old_view = address_space_get_flatview(as); FlatView *new_view = generate_memory_topology(as->root); address_space_update_topology_pass(as, old_view, new_view, false); address_space_update_topology_pass(as, old_view, new_view, true); + qemu_mutex_lock(&flat_view_mutex); + flatview_unref(as->current_map); as->current_map = new_view; - flatview_destroy(old_view); + qemu_mutex_unlock(&flat_view_mutex); + + /* 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. + */ + flatview_unref(old_view); + address_space_update_ioeventfds(as); } @@ -1146,12 +1197,13 @@ void memory_region_sync_dirty_bitmap(MemoryRegion *mr) FlatRange *fr; QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { - FlatView *view = as->current_map; + FlatView *view = address_space_get_flatview(as); FOR_EACH_FLAT_RANGE(fr, view) { if (fr->mr == mr) { MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, log_sync); } } + flatview_unref(view); } } @@ -1203,7 +1255,7 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa AddrRange tmp; MemoryRegionSection section; - view = as->current_map; + view = address_space_get_flatview(as); FOR_EACH_FLAT_RANGE(fr, view) { if (fr->mr == mr) { section = (MemoryRegionSection) { @@ -1229,6 +1281,7 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa } } } + flatview_unref(view); } static void memory_region_update_coalesced_range(MemoryRegion *mr) @@ -1520,7 +1573,7 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr, as = memory_region_to_address_space(root); range = addrrange_make(int128_make64(addr), int128_make64(size)); - view = as->current_map; + view = address_space_get_flatview(as); fr = flatview_lookup(view, range); if (!fr) { return ret; @@ -1541,6 +1594,7 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr, ret.readonly = fr->readonly; memory_region_ref(ret.mr); + flatview_unref(view); return ret; } @@ -1549,10 +1603,11 @@ void address_space_sync_dirty_bitmap(AddressSpace *as) FlatView *view; FlatRange *fr; - view = as->current_map; + view = address_space_get_flatview(as); FOR_EACH_FLAT_RANGE(fr, view) { MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, log_sync); } + flatview_unref(view); } void memory_global_dirty_log_start(void) @@ -1584,7 +1639,7 @@ static void listener_add_address_space(MemoryListener *listener, } } - view = as->current_map; + view = address_space_get_flatview(as); FOR_EACH_FLAT_RANGE(fr, view) { MemoryRegionSection section = { .mr = fr->mr, @@ -1598,6 +1653,7 @@ static void listener_add_address_space(MemoryListener *listener, listener->region_add(listener, §ion); } } + flatview_unref(view); } void memory_listener_register(MemoryListener *listener, AddressSpace *filter) @@ -1631,6 +1687,10 @@ void memory_listener_unregister(MemoryListener *listener) void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name) { + if (QTAILQ_EMPTY(&address_spaces)) { + memory_init(); + } + memory_region_transaction_begin(); as->root = root; as->current_map = g_new(FlatView, 1); @@ -1652,9 +1712,8 @@ void address_space_destroy(AddressSpace *as) memory_region_transaction_commit(); QTAILQ_REMOVE(&address_spaces, as, address_spaces_link); address_space_destroy_dispatch(as); - flatview_destroy(as->current_map); + flatview_unref(as->current_map); g_free(as->name); - g_free(as->current_map); g_free(as->ioeventfds); }
With this change, a FlatView can be used even after a concurrent update has replaced it. Because we do not have RCU, we use a mutex to protect the small critical sections that read/write the as->current_map pointer. Accesses to the FlatView can be done outside the mutex. If a MemoryRegion will be used after the FlatView is unref-ed (or after a MemoryListener callback is returned), a reference has to be added to that MemoryRegion. For example, memory_region_find adds a reference to the MemoryRegion that it returns. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- memory.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 69 insertions(+), 10 deletions(-)