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 <pbonz...@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 <f...@redhat.com>