From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:52704) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QlQCX-0008LC-RN for qemu-devel@nongnu.org; Mon, 25 Jul 2011 14:48:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QlQCW-00023B-QT for qemu-devel@nongnu.org; Mon, 25 Jul 2011 14:48:25 -0400 Received: from mail-yi0-f45.google.com ([209.85.218.45]:41889) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QlQCW-000237-MF for qemu-devel@nongnu.org; Mon, 25 Jul 2011 14:48:24 -0400 Received: by yia25 with SMTP id 25so2894286yia.4 for ; Mon, 25 Jul 2011 11:48:23 -0700 (PDT) Message-ID: <4E2DBA75.5010701@codemonkey.ws> Date: Mon, 25 Jul 2011 13:48:21 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1311602584-23409-1-git-send-email-avi@redhat.com> <1311602584-23409-4-git-send-email-avi@redhat.com> In-Reply-To: <1311602584-23409-4-git-send-email-avi@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 03/23] memory: merge adjacent segments of a single memory region List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org On 07/25/2011 09:02 AM, Avi Kivity wrote: > Simple implementations of memory routers, for example the Cirrus VGA memory banks > or the 440FX PAM registers can generate adjacent memory regions which are contiguous. > Detect these and merge them; this saves kvm memory slots and shortens lookup times. > > Signed-off-by: Avi Kivity > --- > memory.c | 22 ++++++++++++++++++++++ > 1 files changed, 22 insertions(+), 0 deletions(-) > > diff --git a/memory.c b/memory.c > index a569666..339bea3 100644 > --- a/memory.c > +++ b/memory.c > @@ -122,6 +122,27 @@ static void flatview_destroy(FlatView *view) > qemu_free(view->ranges); > } > > +/* Attempt to simplify a view by merging ajacent ranges */ > +static void flatview_simplify(FlatView *view) > +{ > + unsigned i; > + FlatRange *r1, *r2; > + > + for (i = 0; i + 1< view->nr; ++i) { > + r1 =&view->ranges[i]; > + r2 =&view->ranges[i+1]; > + if (addrrange_end(r1->addr) == r2->addr.start > +&& r1->mr == r2->mr > +&& r1->offset_in_region + r1->addr.size == r2->offset_in_region > +&& r1->dirty_log_mask == r2->dirty_log_mask) { > + r1->addr.size += r2->addr.size; > + memmove(r2, r2 + 1, (view->nr - (i + 2)) * sizeof(*r2)); > + --view->nr; > + --i; > + } The --i is pretty subtle. Moving the index variable backwards in a conditional in a for loop is pretty evil :-) I started writing up why this was wrong until I noticed that. I think the following would be more straight forward: i = 0; while (i + 1 < view->nr) { int begin = i, end = i + 1; while (matches(&view->ranges[begin], &view->ranges[end])) { end++; } memmove(...) } Regards, Anthony Liguori > + } > +} > + > /* Render a memory region into the global view. Ranges in @view obscure > * ranges in @mr. > */ > @@ -209,6 +230,7 @@ static FlatView generate_memory_topology(MemoryRegion *mr) > flatview_init(&view); > > render_memory_region(&view, mr, 0, addrrange_make(0, UINT64_MAX)); > + flatview_simplify(&view); > > return view; > }