From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58203) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XbT3D-00012S-8N for qemu-devel@nongnu.org; Tue, 07 Oct 2014 07:35:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XbT37-0003JV-2H for qemu-devel@nongnu.org; Tue, 07 Oct 2014 07:35:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53124) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XbT36-0003JR-OP for qemu-devel@nongnu.org; Tue, 07 Oct 2014 07:35:24 -0400 Date: Tue, 7 Oct 2014 12:35:10 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20141007113509.GK2404@work-vm> References: <1412358473-31398-1-git-send-email-dgilbert@redhat.com> <1412358473-31398-38-git-send-email-dgilbert@redhat.com> <543036A9.3000703@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <543036A9.3000703@redhat.com> Subject: Re: [Qemu-devel] [PATCH v4 37/47] Page request: Consume pages off the post-copy queue List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, lilei@linux.vnet.ibm.com, quintela@redhat.com, cristian.klein@cs.umu.se, qemu-devel@nongnu.org, amit.shah@redhat.com, yanghy@cn.fujitsu.com * Paolo Bonzini (pbonzini@redhat.com) wrote: > Il 03/10/2014 19:47, Dr. David Alan Gilbert (git) ha scritto: > > + /* > > + * Don't break host-page chunks up with queue items > > + * so only unqueue if, > > + * a) The last item came from the queue anyway > > + * b) The last sent item was the last target-page in a host page > > + */ > > + if (last_was_from_queue || (!last_sent_block) || > > Extra parentheses. Fixed. > Is the last_was_from_queue check necessary? Or > would one of the other checks be true anyway if last_was_from_queue is true? if (last_was_from_queue || !last_sent_block || ((last_offset & (hps - 1)) == (hps - TARGET_PAGE_SIZE))) { tmpblock = ram_save_unqueue_page(ms, &tmpoffset, &bitoffset); } The last_was_from_queue is needed. We're going around this loop in Target-page chunks (that correspond to one bit in the migration bitmap) and want to make sure we don't break into the middle of an existing host-page with an entry off the queue. So (going backwards in that if): We can take something from the queue if: a) We just sent the last TP in a HP b) We didn't send anything yet (unlikely) c) The last thing came from the queue anyway (c) is needed to override (a) when we've just sent a TP from the queue but it's not the last TP in the HP that came from the queue; otherwise we'd send the first TP from the queue and resume taking pages from the background scan. > > + /* We're sending this page, and since it's postcopy nothing else > > + * will dirty it, and we must make sure it doesn't get sent again. > > + */ > > + if (!migration_bitmap_clear_dirty(bitoffset << TARGET_PAGE_BITS)) { > > + DPRINTF("%s: Not dirty for postcopy %s/%zx bito=%zx (sent=%d)", > > + __func__, tmpblock->idstr, tmpoffset, bitoffset, > > + test_bit(bitoffset, ms->sentmap)); > > If a DPRINTF occurs in a loop, please change it to a tracepoint. OK, I'll look at that - most of arch_init (and migration) still use DPRINTF. > This function looks like a candidate for cleaning its logic up and/or > splitting it. But it can be done later by the poor soul who will touch > it next. :) Yep, I've already done it once (see 14bcfdc7f - Split ram_save_block). Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK