Re: [Qemu-devel] [PATCH] memory: Don't update all memory region when ioeventfd changed

[Paolo Bonzini <pbonzini@redhat.com>]

Il 08/05/2014 05:47, arei.gonglei@huawei.com ha scritto:
> From: Gonglei <arei.gonglei@huawei.com>
>
> memory mappings don't rely on ioeventfds, there is no need
> to destroy and rebuild them when manipulating ioeventfds,
> otherwise it scarifies performance.
>
> according to testing result, each ioeventfd deleing needs
> about 5ms, within which memory mapping rebuilding needs
> about 4ms. With many Nics and vmchannel in a VM doing migrating,
> there can be many ioeventfds deleting which increasing
> downtime remarkably.
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: Herongguang <herongguang.he@huawei.com>
> ---
>  memory.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/memory.c b/memory.c
> index 3f1df23..2c783f6 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -28,6 +28,7 @@
>
>  static unsigned memory_region_transaction_depth;
>  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
> @@ -786,22 +787,34 @@ void memory_region_transaction_begin(void)
>      ++memory_region_transaction_depth;
>  }
>
> +static void memory_region_clear_pending(void)
> +{
> +    memory_region_update_pending = false;
> +    ioeventfd_update_pending = false;
> +}
> +
>  void memory_region_transaction_commit(void)
>  {
>      AddressSpace *as;
>
>      assert(memory_region_transaction_depth);
>      --memory_region_transaction_depth;
> -    if (!memory_region_transaction_depth && memory_region_update_pending) {
> -        memory_region_update_pending = false;
> -        MEMORY_LISTENER_CALL_GLOBAL(begin, Forward);
> +    if (!memory_region_transaction_depth) {
> +        if (memory_region_update_pending) {
> +            MEMORY_LISTENER_CALL_GLOBAL(begin, Forward);
>
> -        QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> -            address_space_update_topology(as);
> -        }
> +            QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> +                address_space_update_topology(as);
> +            }
>
> -        MEMORY_LISTENER_CALL_GLOBAL(commit, Forward);
> -    }
> +            MEMORY_LISTENER_CALL_GLOBAL(commit, Forward);
> +        } else if (ioeventfd_update_pending) {
> +            QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> +                address_space_update_ioeventfds(as);
> +            }
> +        }
> +        memory_region_clear_pending();
> +   }
>  }
>
>  static void memory_region_destructor_none(MemoryRegion *mr)
> @@ -1373,7 +1386,7 @@ void memory_region_add_eventfd(MemoryRegion *mr,
>      memmove(&mr->ioeventfds[i+1], &mr->ioeventfds[i],
>              sizeof(*mr->ioeventfds) * (mr->ioeventfd_nb-1 - i));
>      mr->ioeventfds[i] = mrfd;
> -    memory_region_update_pending |= mr->enabled;
> +    ioeventfd_update_pending |= mr->enabled;
>      memory_region_transaction_commit();
>  }
>
> @@ -1406,7 +1419,7 @@ void memory_region_del_eventfd(MemoryRegion *mr,
>      --mr->ioeventfd_nb;
>      mr->ioeventfds = g_realloc(mr->ioeventfds,
>                                    sizeof(*mr->ioeventfds)*mr->ioeventfd_nb + 1);
> -    memory_region_update_pending |= mr->enabled;
> +    ioeventfd_update_pending |= mr->enabled;
>      memory_region_transaction_commit();
>  }
>
>

Applied for 2.1, thanks -- but IIUC Chris's patch (if it worked) would 
improve performance even more.  Is this correct?

Paolo