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

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

[Samuel Mendoza-Jonas]

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 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));
+                return -1;
+            }
+        }
+
         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

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

[Dr. David Alan Gilbert]

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

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

[Alexander Graf]

On 19.08.14 08:17, 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 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;

A "need_reset" field in the sPAPR machine with semantics of "we
triggered a reset, so if there's a migration in flight please consider
the full HTAB as invalid and thus reopen the fd to fetch all entries
again" is not exactly obvious to me.

How about something like "htab_save_should_reopen_fd" or an even better
name you might come up with :). Alternatively you could save the last
time stamp of when the HTAB was allocated and compare to that.


Alex