From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58132) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eKOY8-0001zV-Qi for qemu-devel@nongnu.org; Thu, 30 Nov 2017 08:06:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eKOY3-0002Gz-Mh for qemu-devel@nongnu.org; Thu, 30 Nov 2017 08:06:44 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46796) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eKOY3-0002GA-DV for qemu-devel@nongnu.org; Thu, 30 Nov 2017 08:06:39 -0500 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 81E0449016 for ; Thu, 30 Nov 2017 13:06:38 +0000 (UTC) Date: Thu, 30 Nov 2017 13:06:29 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20171130130628.GC3952@work-vm> References: <20171129185026.23632-1-dgilbert@redhat.com> <20171130122245.7e713d2f@redhat.com> <20171130120805.GA3952@work-vm> <20171130134057.1d8ecc3e@redhat.com> <20171130124720.GB3952@work-vm> <20171130135820.69eb5715@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171130135820.69eb5715@redhat.com> Subject: Re: [Qemu-devel] [RFC 0/7] Rework vhost memory region updates List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, maxime.coquelin@redhat.com, mst@redhat.com * Igor Mammedov (imammedo@redhat.com) wrote: > On Thu, 30 Nov 2017 12:47:20 +0000 > "Dr. David Alan Gilbert" wrote: > > > * Igor Mammedov (imammedo@redhat.com) wrote: > > > On Thu, 30 Nov 2017 12:08:06 +0000 > > > "Dr. David Alan Gilbert" wrote: > > > > > > > * Igor Mammedov (imammedo@redhat.com) wrote: > > > > > On Wed, 29 Nov 2017 18:50:19 +0000 > > > > > "Dr. David Alan Gilbert (git)" wrote: > > > > > > > > > > > From: "Dr. David Alan Gilbert" > > > > > > > > > > > > Hi, > > > > > > This is an experimental set that reworks the way the vhost > > > > > > code handles changes in physical address space layout that > > > > > > came from a discussion with Igor. > > > > > Thanks for looking into it. > > > > > > > > > > > > > > > > Instead of updating and trying to merge sections of address > > > > > > space on each add/remove callback, we wait until the commit phase > > > > > > and go through and rebuild a list by walking the Flatview of > > > > > > memory and end up producing an ordered list. > > > > > > We compare the list to the old list to trigger updates. > > > > > > > > > > > > Note, only very lightly tested so far, I'm just trying to see if it's > > > > > > the right shape. > > > > > > > > > > > > Igor, is this what you were intending? > > > > > > > > > > I was thinking about a little less intrusive approach > > > > > > > > > > where vhost_region_add/del are modified to maintain > > > > > sorted by GPA array of mem_sections, vhost_dev::mem is dropped > > > > > altogether and vhost_memory_region array is build/used/freed > > > > > on every vhost_commit(). > > > > > Maintaining sorted array should roughly cost us O(2 log n) if > > > > > binary search is used. > > > > > > > > > > However I like your idea with iterator even more as it have > > > > > potential to make it even faster O(n) if we get rid of > > > > > quadratic and relatively complex vhost_update_compare_list(). > > > > > > > > Note vhost_update_compare_list is complex, > > > > but it is O(n) - it's > > > > got nested loops, but the inner loop moves forward and oldi never > > > > gets reset back to zero. > > > While skimming through patches I've overlooked it. > > > > > > Anyways, > > > why memcmp(old_arr, new_arr) is not sufficient > > > to detect a change in memory map? > > > > It tells you that you've got a change, but doesn't give > > the start/end of the range that's changed, and those > > are used by vhost_commit to limit the work of > > vhost_verify_ring_mappings. > Isn't memmap list a sorted and > dev->mem_changed_[start|end]_addr are the lowest|highest > addresses of whole map? > > If it's, so wouldn't getting values directly from > the 1st/last entries of array be sufficient? THat wasn't my understanding from the existing code; my understanding was that changed_start_addr is set by vhost_region_add->vhost_set_memory when a new region is added (or removed) and is set to the limit of the section added. But perhaps I'm misunderstanding. (The logic in vhost_verify_ring_mappings doesn't make sense to me either though; if vhost_verify_ring_part_mapping returns 0 on success, why is it doing if (!r) { break; } surely it should be if (r) { break; }) Dave > > > > > > Dave > > > > > > > > > > > Pls, see comments on individual patches. > > > > > > > > Thanks; I have fixed a couple of bugs since I posted, so I'm > > > > more interested in structure/shape. Any good ideas how to test > > > > it are welcome. > > > > > > > > Dave > > > > > > > > > > > > > > > Dave > > > > > > > > > > > > Dr. David Alan Gilbert (7): > > > > > > memory: address_space_iterate > > > > > > vhost: Move log_dirty check > > > > > > vhost: New memory update functions > > > > > > vhost: update_mem_cb implementation > > > > > > vhost: Compare new and old memory lists > > > > > > vhost: Copy updated region data into device state > > > > > > vhost: Remove vhost_set_memory and children > > > > > > > > > > > > hw/virtio/trace-events | 8 + > > > > > > hw/virtio/vhost.c | 424 ++++++++++++++++++++++--------------------------- > > > > > > include/exec/memory.h | 23 +++ > > > > > > memory.c | 22 +++ > > > > > > 4 files changed, 241 insertions(+), 236 deletions(-) > > > > > > > > > > > > > > > -- > > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK