Re: [PATCH v7 1/6] memory: Reference as->current_map directly in memory commit

[David Edmondson]

Chuang Xu <xuchuangxclwt@bytedance.com> writes:

> From: Peter Xu <peterx@redhat.com>
>
> Calling RCU variance of address_space_get|to_flatview() during memory

"variants" rather than "variance", perhaps?

> commit (flatview updates, triggering memory listeners, or updating
> ioeventfds, etc.) is not 100% accurate, because commit() requires BQL
> rather than RCU read lock, so the context exclusively owns current_map and
> can be directly referenced.
>
> Neither does it need a refcount to current_map because it cannot be freed
> from under the caller.
>
> Add address_space_get_flatview_raw() for the case where the context holds
> BQL rather than RCU read lock and use it across the core memory updates,
> Drop the extra refcounts on FlatView*.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  softmmu/memory.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 4699ba55ec..a992a365d9 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -61,6 +61,13 @@ struct AddrRange {
>      Int128 size;
>  };
>  
> +/* Called with BQL held */
> +static inline FlatView *address_space_to_flatview_raw(AddressSpace *as)
> +{
> +    assert(qemu_mutex_iothread_locked());
> +    return as->current_map;
> +}
> +
>  static AddrRange addrrange_make(Int128 start, Int128 size)
>  {
>      return (AddrRange) { start, size };
> @@ -155,7 +162,7 @@ enum ListenerDirection { Forward, Reverse };
>  #define MEMORY_LISTENER_UPDATE_REGION(fr, as, dir, callback, _args...)  \
>      do {                                                                \
>          MemoryRegionSection mrs = section_from_flat_range(fr,           \
> -                address_space_to_flatview(as));                         \
> +                address_space_to_flatview_raw(as));                     \
>          MEMORY_LISTENER_CALL(as, callback, dir, &mrs, ##_args);         \
>      } while(0)
>  
> @@ -753,6 +760,7 @@ static FlatView *generate_memory_topology(MemoryRegion 
> *mr)
>  }
>  
>  static void address_space_add_del_ioeventfds(AddressSpace *as,
> +                                             FlatView *view,
>                                               MemoryRegionIoeventfd *fds_new,
>                                               unsigned fds_new_nb,
>                                               MemoryRegionIoeventfd *fds_old,
> @@ -774,7 +782,7 @@ static void address_space_add_del_ioeventfds(AddressSpace 
> *as,
>                                                    &fds_new[inew]))) {
>              fd = &fds_old[iold];
>              section = (MemoryRegionSection) {
> -                .fv = address_space_to_flatview(as),
> +                .fv = view,
>                  .offset_within_address_space = int128_get64(fd->addr.start),
>                  .size = fd->addr.size,
>              };
> @@ -787,7 +795,7 @@ static void address_space_add_del_ioeventfds(AddressSpace 
> *as,
>                                                           &fds_old[iold]))) {
>              fd = &fds_new[inew];
>              section = (MemoryRegionSection) {
> -                .fv = address_space_to_flatview(as),
> +                .fv = view,
>                  .offset_within_address_space = int128_get64(fd->addr.start),
>                  .size = fd->addr.size,
>              };
> @@ -833,7 +841,7 @@ static void address_space_update_ioeventfds(AddressSpace 
> *as)
>      ioeventfd_max = QEMU_ALIGN_UP(as->ioeventfd_nb, 4);
>      ioeventfds = g_new(MemoryRegionIoeventfd, ioeventfd_max);
>  
> -    view = address_space_get_flatview(as);
> +    view = address_space_to_flatview_raw(as);
>      FOR_EACH_FLAT_RANGE(fr, view) {
>          for (i = 0; i < fr->mr->ioeventfd_nb; ++i) {
>              tmp = addrrange_shift(fr->mr->ioeventfds[i].addr,
> @@ -852,13 +860,12 @@ static void 
> address_space_update_ioeventfds(AddressSpace *as)
>          }
>      }
>  
> -    address_space_add_del_ioeventfds(as, ioeventfds, ioeventfd_nb,
> +    address_space_add_del_ioeventfds(as, view, ioeventfds, ioeventfd_nb,
>                                       as->ioeventfds, as->ioeventfd_nb);
>  
>      g_free(as->ioeventfds);
>      as->ioeventfds = ioeventfds;
>      as->ioeventfd_nb = ioeventfd_nb;
> -    flatview_unref(view);
>  }
>  
>  /*
> @@ -1026,7 +1033,7 @@ static void flatviews_reset(void)
>  
>  static void address_space_set_flatview(AddressSpace *as)
>  {
> -    FlatView *old_view = address_space_to_flatview(as);
> +    FlatView *old_view = address_space_to_flatview_raw(as);
>      MemoryRegion *physmr = memory_region_get_flatview_root(as->root);
>      FlatView *new_view = g_hash_table_lookup(flat_views, physmr);
>  
> @@ -2979,8 +2986,7 @@ static void listener_add_address_space(MemoryListener 
> *listener,
>              listener->log_global_start(listener);
>          }
>      }
> -
> -    view = address_space_get_flatview(as);
> +    view = address_space_to_flatview_raw(as);
>      FOR_EACH_FLAT_RANGE(fr, view) {
>          MemoryRegionSection section = section_from_flat_range(fr, view);
>  
> @@ -2994,7 +3000,6 @@ static void listener_add_address_space(MemoryListener 
> *listener,
>      if (listener->commit) {
>          listener->commit(listener);
>      }
> -    flatview_unref(view);
>  }
>  
>  static void listener_del_address_space(MemoryListener *listener,
> @@ -3006,7 +3011,7 @@ static void listener_del_address_space(MemoryListener 
> *listener,
>      if (listener->begin) {
>          listener->begin(listener);
>      }
> -    view = address_space_get_flatview(as);
> +    view = address_space_to_flatview_raw(as);
>      FOR_EACH_FLAT_RANGE(fr, view) {
>          MemoryRegionSection section = section_from_flat_range(fr, view);
>  
> @@ -3020,7 +3025,6 @@ static void listener_del_address_space(MemoryListener 
> *listener,
>      if (listener->commit) {
>          listener->commit(listener);
>      }
> -    flatview_unref(view);
>  }
>  
>  void memory_listener_register(MemoryListener *listener, AddressSpace *as)
> -- 
> 2.20.1
-- 
Leaves are falling all around, it's time I was on my way.