From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51467) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TUfCH-0008G2-Jd for qemu-devel@nongnu.org; Sat, 03 Nov 2012 10:59:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TUfCF-0005Ge-MR for qemu-devel@nongnu.org; Sat, 03 Nov 2012 10:59:41 -0400 Received: from e23smtp08.au.ibm.com ([202.81.31.141]:54352) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TUfCF-0005BS-43 for qemu-devel@nongnu.org; Sat, 03 Nov 2012 10:59:39 -0400 Received: from /spool/local by e23smtp08.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sun, 4 Nov 2012 00:58:36 +1000 Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id qA3En7bK15466692 for ; Sun, 4 Nov 2012 01:49:07 +1100 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id qA3ExPbS024085 for ; Sun, 4 Nov 2012 01:59:26 +1100 Date: Sun, 4 Nov 2012 02:00:54 +1100 From: David Gibson Message-ID: <20121103150054.GT27695@truffula.fritz.box> References: <1351654988-13165-1-git-send-email-david@gibson.dropbear.id.au> <509106A0.7020400@redhat.com> <20121102031508.GN27695@truffula.fritz.box> <87625oi84n.fsf@elfo.mitica> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87625oi84n.fsf@elfo.mitica> Subject: Re: [Qemu-devel] [PATCH] Fix off-by-1 error in RAM migration code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: Orit Wasserman , aliguori@us.ibm.com, qemu-devel@nongnu.org On Fri, Nov 02, 2012 at 11:58:32AM +0100, Juan Quintela wrote: > David Gibson wrote: > > On Wed, Oct 31, 2012 at 01:08:16PM +0200, Orit Wasserman wrote: > >> On 10/31/2012 05:43 AM, David Gibson wrote: > >> > The code for migrating (or savevm-ing) memory pages starts off by creating > >> > a dirty bitmap and filling it with 1s. Except, actually, because bit > >> > addresses are 0-based it fills every bit except bit 0 with 1s and puts an > >> > extra 1 beyond the end of the bitmap, potentially corrupting unrelated > >> > memory. Oops. This patch fixes it. > >> > > >> > Signed-off-by: David Gibson > >> > --- > >> > arch_init.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/arch_init.c b/arch_init.c > >> > index e6effe8..b75a4c5 100644 > >> > --- a/arch_init.c > >> > +++ b/arch_init.c > >> > @@ -568,7 +568,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > >> > int64_t ram_pages = last_ram_offset() >> TARGET_PAGE_BITS; > >> > > >> > migration_bitmap = bitmap_new(ram_pages); > >> > - bitmap_set(migration_bitmap, 1, ram_pages); > >> > + bitmap_set(migration_bitmap, 0, ram_pages); > >> > migration_dirty_pages = ram_pages; > >> > > >> > bytes_transferred = 0; > >> > > >> You are correct, good catch. > >> Reviewed-by: Orit Wasserman > > > > Juan, > > > > Sorry, forgot to CC you on the original mailing here, which I should > > have done. This is a serious bug in the migration code and we should > > apply to mainline ASAP. > > Reviewed-by: Juan Quintela > > Good catch, I missunderstood the function when fixing a different bug, > and never undrestood why it fixed it. Actually.. it just occurred to me that I think there has to be another bug here somewhere.. I haven't actually observed any effects from the memory corruption - though it's certainly a real bug. I found this because another effect of this bug is that migration_dirty_pages count was set to 1 more than the actual number of dirty bits in the bitmap. That meant the dirty pages count was never reaching zero and so the migration/savevm never terminated. Except.. that every so often the migration *did* terminate (maybe 1 time in 5). Also I kind of hope somebody would have noticed this earlier if migrations never terminated on x86 too. But as far as I can tell, if initially mismatched like this it ought to be impossible for the dirty page count to ever reach zero. Which suggests there is another bug with the dirty count tracking :(. It's possible the memory corruption could account for this, of course - since that in theory at least, could have almost any strange effect on the program's behavior. But that doesn't seem particularly likely to me. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson