From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37051) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJzey-0007zP-CR for qemu-devel@nongnu.org; Wed, 20 Aug 2014 02:46:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XJzeq-0001Pl-Rc for qemu-devel@nongnu.org; Wed, 20 Aug 2014 02:46:16 -0400 Received: from mail-pd0-f181.google.com ([209.85.192.181]:33600) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJzeq-0001Pg-KL for qemu-devel@nongnu.org; Wed, 20 Aug 2014 02:46:08 -0400 Received: by mail-pd0-f181.google.com with SMTP id g10so10826342pdj.26 for ; Tue, 19 Aug 2014 23:46:00 -0700 (PDT) Message-ID: <53F4441D.406@ozlabs.ru> Date: Wed, 20 Aug 2014 16:45:49 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1408500822-12890-1-git-send-email-sam.mj@au1.ibm.com> In-Reply-To: <1408500822-12890-1-git-send-email-sam.mj@au1.ibm.com> Content-Type: text/plain; charset=koi8-r Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V4] spapr: Fix stale HTAB during live migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Samuel Mendoza-Jonas , qemu-devel@nongnu.org, qemu-ppc@nongnu.org Cc: dgilbert@redhat.com On 08/20/2014 12:13 PM, Samuel Mendoza-Jonas 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. > > Signed-off-by: Samuel Mendoza-Jonas > --- > Changes in v4: Readability: need_reset to htab_fd_stale > Add spapr_check_htab_fd() and use error_report() > 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 | 37 +++++++++++++++++++++++++++++++++++++ > include/hw/ppc/spapr.h | 1 + > 2 files changed, 38 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 3a6d26d..68f97a9 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -997,6 +997,11 @@ 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->htab_fd_stale = true; > + } > } else { > if (!spapr->htab) { > /* Allocate an htab if we don't yet have one */ > @@ -1014,6 +1019,27 @@ static void spapr_reset_htab(sPAPREnvironment *spapr) > } > } > > +/* A guest reset will cause spapr->htab_fd to become stale if being used. > + * Reopen the file descriptor to make sure the whole HTAB is properly read. > + */ > +static int spapr_check_htab_fd(sPAPREnvironment *spapr) > +{ > + int rc = 0; > + > + if (atomic_cmpxchg(&spapr->htab_fd_stale, true, false) == true) { I was wrong about xchg :) You have to loop atomic_cmpxchg() if it returns false. Second, "if" and "== true" does not look nice, I'd drop "== true". Also this is (probably?) missing a memory barrier between setting and clearing @htab_fd_stale. Paul suggested using pthread_mutex_lock() + pthread_mutex_unlock() where we set and clear the flag as we do not expect millions of those and this is way simpler. > + close(spapr->htab_fd); > + spapr->htab_fd = kvmppc_get_htab_fd(false); > + > + if (spapr->htab_fd < 0) { > + error_report("Unable to open fd for reading hash table from KVM: " > + "%s", strerror(errno)); > + rc = -1;; Double semicolon. > + } > + } > + > + return rc; > +} > + > static void ppc_spapr_reset(void) > { > PowerPCCPU *first_ppc_cpu; > @@ -1156,6 +1182,7 @@ static int htab_save_setup(QEMUFile *f, void *opaque) > } else { > assert(kvm_enabled()); > > + spapr->htab_fd_stale = 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 +1336,11 @@ static int htab_save_iterate(QEMUFile *f, void *opaque) > if (!spapr->htab) { > assert(kvm_enabled()); > > + rc = spapr_check_htab_fd(spapr); > + if (rc < 0) { > + return rc; > + } > + > rc = kvmppc_save_htab(f, spapr->htab_fd, > MAX_KVM_BUF_SIZE, MAX_ITERATION_NS); > if (rc < 0) { > @@ -1340,6 +1372,11 @@ static int htab_save_complete(QEMUFile *f, void *opaque) > > assert(kvm_enabled()); > > + rc = spapr_check_htab_fd(spapr); > + if (rc < 0) { > + return rc; > + } > + > 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..0421d9a 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 htab_fd_stale; > > /* state for Dynamic Reconfiguration Connectors */ > sPAPRDrcEntry drc_table[SPAPR_DRC_TABLE_SIZE]; > -- Alexey