Paolo Bonzini <pbonz...@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 <pbonz...@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
