From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53745) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJehl-0003eY-SB for qemu-devel@nongnu.org; Tue, 19 Aug 2014 04:23:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XJehe-0006rv-2P for qemu-devel@nongnu.org; Tue, 19 Aug 2014 04:23:45 -0400 Date: Tue, 19 Aug 2014 09:23:27 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20140819082326.GB2383@work-vm> References: <1408429031-1716-1-git-send-email-sam.mj@au1.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1408429031-1716-1-git-send-email-sam.mj@au1.ibm.com> Subject: Re: [Qemu-devel] [PATCH V3] spapr: Fix stale HTAB during live migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Samuel Mendoza-Jonas Cc: aik@ozlabs.ru, qemu-ppc@nongnu.org, qemu-devel@nongnu.org * Samuel Mendoza-Jonas (sam.mj@au1.ibm.com) wrote: > If a guest reboots during a running migration, changes to the > hash page table are not necessarily updated on the destination. > Opening a new file descriptor to the HTAB forces the migration > handler to resend the entire table. Yes I think that's safe. > Signed-off-by: Samuel Mendoza-Jonas > --- > Changes in v3: Pointed out by David, htab_save_iterate could > potentially try to read before htab_fd is open again. > Leave opening the fd to the functions trying to read. > Changes in v2: Forgot check on kvmppc_get_htab_fd return value > hw/ppc/spapr.c | 25 +++++++++++++++++++++++++ > include/hw/ppc/spapr.h | 1 + > 2 files changed, 26 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 3a6d26d..5b41318 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -997,6 +997,10 @@ static void spapr_reset_htab(sPAPREnvironment *spapr) > /* Kernel handles htab, we don't need to allocate one */ > spapr->htab_shift = shift; > kvmppc_kern_htab = true; > + > + /* Check if we are overlapping a migration */ > + if (spapr->htab_fd > 0) > + spapr->need_reset = true; > } else { > if (!spapr->htab) { > /* Allocate an htab if we don't yet have one */ > @@ -1156,6 +1160,7 @@ static int htab_save_setup(QEMUFile *f, void *opaque) > } else { > assert(kvm_enabled()); > > + spapr->need_reset = false; > spapr->htab_fd = kvmppc_get_htab_fd(false); > if (spapr->htab_fd < 0) { > fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n", > @@ -1309,6 +1314,16 @@ static int htab_save_iterate(QEMUFile *f, void *opaque) > if (!spapr->htab) { > assert(kvm_enabled()); > > + if (atomic_cmpxchg(&spapr->need_reset, true, false) == true) { > + close(spapr->htab_fd); > + spapr->htab_fd = kvmppc_get_htab_fd(false); > + if (spapr->htab_fd < 0) { > + fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n", > + strerror(errno)); Either perror or error_report() with the strerror would seem better. > + return -1; > + } > + } > + Why not make a little function for this; it seems a bad idea to have two copies of it. Also, add a comment saying why you're reopening it. Dave > rc = kvmppc_save_htab(f, spapr->htab_fd, > MAX_KVM_BUF_SIZE, MAX_ITERATION_NS); > if (rc < 0) { > @@ -1340,6 +1355,16 @@ static int htab_save_complete(QEMUFile *f, void *opaque) > > assert(kvm_enabled()); > > + if (atomic_cmpxchg(&spapr->need_reset, true, false) == true) { > + close(spapr->htab_fd); > + spapr->htab_fd = kvmppc_get_htab_fd(false); > + if (spapr->htab_fd < 0) { > + fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n", > + strerror(errno)); > + return -1; > + } > + } > + > rc = kvmppc_save_htab(f, spapr->htab_fd, MAX_KVM_BUF_SIZE, -1); > if (rc < 0) { > return rc; > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 0c2e3c5..9ab9827 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -71,6 +71,7 @@ typedef struct sPAPREnvironment { > int htab_save_index; > bool htab_first_pass; > int htab_fd; > + bool need_reset; > > /* state for Dynamic Reconfiguration Connectors */ > sPAPRDrcEntry drc_table[SPAPR_DRC_TABLE_SIZE]; > -- > 1.9.3 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK