From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58219) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xooa6-0003Af-Nt for qemu-devel@nongnu.org; Thu, 13 Nov 2014 02:12:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XooZz-00077O-55 for qemu-devel@nongnu.org; Thu, 13 Nov 2014 02:12:38 -0500 Received: from mail-pa0-f53.google.com ([209.85.220.53]:52778) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XooZy-00077C-TF for qemu-devel@nongnu.org; Thu, 13 Nov 2014 02:12:31 -0500 Received: by mail-pa0-f53.google.com with SMTP id kx10so14736984pab.12 for ; Wed, 12 Nov 2014 23:12:30 -0800 (PST) Message-ID: <546459D6.1040706@ozlabs.ru> Date: Thu, 13 Nov 2014 18:12:22 +1100 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1415168221-2324-1-git-send-email-sam.mj@au1.ibm.com> <1415168221-2324-3-git-send-email-sam.mj@au1.ibm.com> In-Reply-To: <1415168221-2324-3-git-send-email-sam.mj@au1.ibm.com> Content-Type: text/plain; charset=koi8-r Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] spapr: Fix stale HTAB during live migration (TCG) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Samuel Mendoza-Jonas , qemu-ppc@nongnu.org, qemu-devel@nongnu.org On 11/05/2014 05:17 PM, Samuel Mendoza-Jonas wrote: > If a TCG guest reboots during a running migration HTAB entries are not > marked dirty, and the destination boots with an invalid HTAB. > > When a reboot occurs reset the state of HTAB migration, and explicitly > inform the destination of invalid entries. > > Signed-off-by: Samuel Mendoza-Jonas > --- > hw/ppc/spapr.c | 59 +++++++++++++++++++++++++++++++++++--------------- > include/hw/ppc/spapr.h | 1 + > 2 files changed, 42 insertions(+), 18 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 1610c28..9f419e8 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -829,26 +829,30 @@ static void spapr_reset_htab(sPAPREnvironment *spapr) > > shift = kvmppc_reset_htab(spapr->htab_shift); > > + pthread_mutex_lock(&spapr->htab_mutex); > if (shift > 0) { > /* Kernel handles htab, we don't need to allocate one */ > spapr->htab_shift = shift; > kvmppc_kern_htab = true; > > /* Tell readers to update their file descriptor */ > - pthread_mutex_lock(&spapr->htab_mutex); > if (spapr->htab_fd > 0) { > spapr->htab_fd_stale = true; > } > - pthread_mutex_unlock(&spapr->htab_mutex); > } else { > if (!spapr->htab) { > /* Allocate an htab if we don't yet have one */ > spapr->htab = qemu_memalign(HTAB_SIZE(spapr), HTAB_SIZE(spapr)); > + } else { > + spapr->htab_mig_full = true; > + spapr->htab_first_pass = true; > + spapr->htab_save_index = 0; > } > > /* And clear it */ > memset(spapr->htab, 0, HTAB_SIZE(spapr)); > } > + pthread_mutex_unlock(&spapr->htab_mutex); The pthread_mutex_(un)lock things from the chunk above should go to the previous patch. > > /* Update the RMA size if necessary */ > if (spapr->vrma_adjust) { > @@ -1019,6 +1023,7 @@ static int htab_save_setup(QEMUFile *f, void *opaque) > pthread_mutex_lock(&spapr->htab_mutex); > spapr->htab_fd = kvmppc_get_htab_fd(false); > spapr->htab_fd_stale = false; > + spapr->htab_mig_full = false; > pthread_mutex_unlock(&spapr->htab_mutex); > if (spapr->htab_fd < 0) { > fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n", > @@ -1034,6 +1039,7 @@ static int htab_save_setup(QEMUFile *f, void *opaque) > static void htab_save_first_pass(QEMUFile *f, sPAPREnvironment *spapr, > int64_t max_ns) > { > + bool final = max_ns < 0; > int htabslots = HTAB_SIZE(spapr) / HASH_PTE_SIZE_64; > int index = spapr->htab_save_index; > int64_t starttime = qemu_clock_get_ns(QEMU_CLOCK_REALTIME); > @@ -1041,33 +1047,40 @@ static void htab_save_first_pass(QEMUFile *f, sPAPREnvironment *spapr, > assert(spapr->htab_first_pass); > > do { > - int chunkstart; > + int chunkstart, invalidstart; > > - /* Consume invalid HPTEs */ > - while ((index < htabslots) > - && !HPTE_VALID(HPTE(spapr->htab, index))) { Nit: in most places in qemu "&&" would be in the end of the previos line :) > + chunkstart = index; > + /* Consume valid HPTEs */ > + while ((index < htabslots && (index - chunkstart < USHRT_MAX)) > + && HPTE_VALID(HPTE(spapr->htab, index))) { > index++; > CLEAN_HPTE(HPTE(spapr->htab, index)); > } > > - /* Consume valid HPTEs */ > - chunkstart = index; > - while ((index < htabslots) > - && HPTE_VALID(HPTE(spapr->htab, index))) { > + invalidstart = index; > + /* Consume invalid HPTEs */ > + while ((index < htabslots && (index - invalidstart < USHRT_MAX)) > + && !HPTE_VALID(HPTE(spapr->htab, index))) { > index++; > CLEAN_HPTE(HPTE(spapr->htab, index)); > } > > - if (index > chunkstart) { > - int n_valid = index - chunkstart; > + /* Avoid writing an end marker (0,0,0) */ > + if (index > chunkstart > + && !(chunkstart == invalidstart && !spapr->htab_mig_full)) { > + int n_valid = invalidstart - chunkstart; > + /* If a reset has occured we must explicitly overwrite the HTAB > + * of the destination */ > + int n_invalid = spapr->htab_mig_full ? index - invalidstart : 0; > > qemu_put_be32(f, chunkstart); > qemu_put_be16(f, n_valid); > - qemu_put_be16(f, 0); > + qemu_put_be16(f, n_invalid); > qemu_put_buffer(f, HPTE(spapr->htab, chunkstart), > HASH_PTE_SIZE_64 * n_valid); > > - if ((qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - starttime) > max_ns) { > + if ((qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - starttime) > max_ns > + && !final) { > break; > } > } > @@ -1182,10 +1195,14 @@ static int htab_save_iterate(QEMUFile *f, void *opaque) > if (rc < 0) { > return rc; > } > - } else if (spapr->htab_first_pass) { > - htab_save_first_pass(f, spapr, MAX_ITERATION_NS); > } else { > - rc = htab_save_later_pass(f, spapr, MAX_ITERATION_NS); > + pthread_mutex_lock(&spapr->htab_mutex); > + if (spapr->htab_first_pass) { > + htab_save_first_pass(f, spapr, MAX_ITERATION_NS); > + } else { > + rc = htab_save_later_pass(f, spapr, MAX_ITERATION_NS); > + } > + pthread_mutex_unlock(&spapr->htab_mutex); > } > > /* End marker */ > @@ -1220,7 +1237,13 @@ static int htab_save_complete(QEMUFile *f, void *opaque) > close(spapr->htab_fd); > spapr->htab_fd = -1; > } else { > - htab_save_later_pass(f, spapr, -1); > + pthread_mutex_lock(&spapr->htab_mutex); > + if (spapr->htab_first_pass) { > + htab_save_first_pass(f, spapr, -1); > + } else { > + htab_save_later_pass(f, spapr, -1); > + } > + pthread_mutex_unlock(&spapr->htab_mutex); > } > > /* End marker */ > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 5e29bec..ee95459 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -38,6 +38,7 @@ typedef struct sPAPREnvironment { > bool htab_first_pass; > int htab_fd; > bool htab_fd_stale; > + bool htab_mig_full; Not sure what @htab_mig_full stands for exactly. Please explain it in the commit log. > pthread_mutex_t htab_mutex; > } sPAPREnvironment; > > -- Alexey