Re: [Qemu-devel] [PATCH 06/15] memory: protect current_map by RCU

[Fam Zheng <famz@redhat.com>]

On Thu, 01/22 15:47, Paolo Bonzini wrote:
> Replace the flat_view_mutex by RCU, avoiding futex contention for
> dataplane on large systems and many iothreads.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/exec/memory.h |  5 +++++
>  memory.c              | 54 ++++++++++++++++++++++-----------------------------
>  2 files changed, 28 insertions(+), 31 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 0cd96b1..06ffa1d 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -33,6 +33,7 @@
>  #include "qemu/notify.h"
>  #include "qapi/error.h"
>  #include "qom/object.h"
> +#include "qemu/rcu.h"
>  
>  #define MAX_PHYS_ADDR_SPACE_BITS 62
>  #define MAX_PHYS_ADDR            (((hwaddr)1 << MAX_PHYS_ADDR_SPACE_BITS) - 1)
> @@ -207,9 +208,13 @@ struct MemoryListener {
>   */
>  struct AddressSpace {
>      /* All fields are private. */
> +    struct rcu_head rcu;
>      char *name;
>      MemoryRegion *root;
> +
> +    /* Accessed via RCU.  */
>      struct FlatView *current_map;
> +
>      int ioeventfd_nb;
>      struct MemoryRegionIoeventfd *ioeventfds;
>      struct AddressSpaceDispatch *dispatch;
> diff --git a/memory.c b/memory.c
> index 8c3d8c0..a844ced 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -33,26 +33,12 @@ static bool memory_region_update_pending;
>  static bool ioeventfd_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;
>  
>  /*
> @@ -242,6 +228,7 @@ struct FlatRange {
>   * order.
>   */
>  struct FlatView {
> +    struct rcu_head rcu;
>      unsigned ref;
>      FlatRange *ranges;
>      unsigned nr;
> @@ -654,10 +641,10 @@ static FlatView *address_space_get_flatview(AddressSpace *as)
>  {
>      FlatView *view;
>  
> -    qemu_mutex_lock(&flat_view_mutex);
> -    view = as->current_map;
> +    rcu_read_lock();
> +    view = atomic_rcu_read(&as->current_map);
>      flatview_ref(view);
> -    qemu_mutex_unlock(&flat_view_mutex);
> +    rcu_read_unlock();
>      return view;
>  }
>  
> @@ -766,10 +753,9 @@ static void address_space_update_topology(AddressSpace *as)
>      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;
> -    qemu_mutex_unlock(&flat_view_mutex);
> +    /* Writes are protected by the BQL.  */
> +    atomic_rcu_set(&as->current_map, new_view);
> +    call_rcu(old_view, flatview_unref, rcu);
>  
>      /* Note that all the old MemoryRegions are still alive up to this
>       * point.  This relieves most MemoryListeners from the need to
> @@ -1957,10 +1943,6 @@ 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);
> @@ -1974,15 +1956,10 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
>      memory_region_transaction_commit();
>  }
>  
> -void address_space_destroy(AddressSpace *as)
> +static void do_address_space_destroy(AddressSpace *as)
>  {
>      MemoryListener *listener;
>  
> -    /* 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_destroy_dispatch(as);
>  
>      QTAILQ_FOREACH(listener, &memory_listeners, link) {
> @@ -1994,6 +1971,21 @@ void address_space_destroy(AddressSpace *as)
>      g_free(as->ioeventfds);
>  }
>  
> +void address_space_destroy(AddressSpace *as)
> +{
> +    /* 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);
> +
> +    /* At this point, as->dispatch and as->current_map are dummy
> +     * entries that the guest should never use.  Wait for the old
> +     * values to expire before freeing the data.
> +     */
> +    call_rcu(as, do_address_space_destroy, rcu);
> +}
> +
>  bool io_mem_read(MemoryRegion *mr, hwaddr addr, uint64_t *pval, unsigned size)
>  {
>      return memory_region_dispatch_read(mr, addr, pval, size);
> -- 
> 1.8.3.1
> 
> 
Reviewed-by: Fam Zheng <famz@redhat.com>