From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43601) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WiNl6-00058i-P3 for qemu-devel@nongnu.org; Thu, 08 May 2014 08:49:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WiNl0-0005ZL-TQ for qemu-devel@nongnu.org; Thu, 08 May 2014 08:49:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:28587) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WiNl0-0005ZD-Lz for qemu-devel@nongnu.org; Thu, 08 May 2014 08:49:02 -0400 Message-ID: <536B7D32.4060909@redhat.com> Date: Thu, 08 May 2014 14:48:50 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1399520852-140212-1-git-send-email-arei.gonglei@huawei.com> In-Reply-To: <1399520852-140212-1-git-send-email-arei.gonglei@huawei.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] memory: Don't update all memory region when ioeventfd changed List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: arei.gonglei@huawei.com, qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, weidong.huang@huawei.com, mst@redhat.com, chris.friesen@windriver.com, Herongguang , rth@twiddle.net Il 08/05/2014 05:47, arei.gonglei@huawei.com ha scritto: > From: Gonglei > > 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 > Signed-off-by: Herongguang > --- > 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