Re: [Qemu-devel] [PATCH 03/30] memory: add reference counting to FlatView

[Anthony Liguori <anthony@codemonkey.ws>]

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, &section);
>          }
>      }
> +    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