Re: [Qemu-devel] [PATCH V4] spapr: Fix stale HTAB during live migration

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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 <sam.mj@au1.ibm.com>
> ---
> 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