Re: [Qemu-devel] [PATCH v2 8/8] migration/ram: ensure write persistence on loading xbzrle pages to PMEM

[Dr. David Alan Gilbert]

* Haozhong Zhang (haozhong.zh...@intel.com) wrote:
> When loading a xbzrle encoded page to persistent memory, load the data
> via libpmem function pmem_memcpy_nodrain() instead of memcpy().
> Combined with a call to pmem_drain() at the end of memory loading, we
> can guarantee those xbzrle encoded pages are persistently loaded to PMEM.
> 
> Signed-off-by: Haozhong Zhang 
> ---
>  migration/ram.c| 15 ++-
>  migration/xbzrle.c | 20 ++--
>  migration/xbzrle.h |  1 +
>  3 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 924d2b9537..87f977617d 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2388,10 +2388,10 @@ static void ram_save_pending(QEMUFile *f, void 
> *opaque, uint64_t max_size,
>  }
>  }
>  
> -static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
> +static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host, bool 
> is_pmem)
>  {
>  unsigned int xh_len;
> -int xh_flags;
> +int xh_flags, rc;
>  uint8_t *loaded_data;
>  
>  /* extract RLE header */
> @@ -2413,8 +2413,13 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, 
> void *host)
>  qemu_get_buffer_in_place(f, _data, xh_len);
>  
>  /* decode RLE */
> -if (xbzrle_decode_buffer(loaded_data, xh_len, host,
> - TARGET_PAGE_SIZE) == -1) {
> +if (!is_pmem) {
> +rc = xbzrle_decode_buffer(loaded_data, xh_len, host, 
> TARGET_PAGE_SIZE);
> +} else {
> +rc = xbzrle_decode_buffer_to_pmem(loaded_data, xh_len, host,
> +  TARGET_PAGE_SIZE);
> +}
> +if (rc == -1) {
>  error_report("Failed to load XBZRLE page - decode error!");
>  return -1;
>  }
> @@ -2974,7 +2979,7 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>  break;
>  
>  case RAM_SAVE_FLAG_XBZRLE:
> -if (load_xbzrle(f, addr, host) < 0) {
> +if (load_xbzrle(f, addr, host, is_pmem) < 0) {
>  error_report("Failed to decompress XBZRLE page at "
>   RAM_ADDR_FMT, addr);
>  ret = -EINVAL;
> diff --git a/migration/xbzrle.c b/migration/xbzrle.c
> index 1ba482ded9..499d8e1bfb 100644
> --- a/migration/xbzrle.c
> +++ b/migration/xbzrle.c
> @@ -12,6 +12,7 @@
>   */
>  #include "qemu/osdep.h"
>  #include "qemu/cutils.h"
> +#include "qemu/pmem.h"
>  #include "xbzrle.h"
>  
>  /*
> @@ -126,7 +127,8 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t 
> *new_buf, int slen,
>  return d;
>  }
>  
> -int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t *dst, int dlen)
> +static int xbzrle_decode_buffer_common(uint8_t *src, int slen, uint8_t *dst,
> +   int dlen, bool is_pmem)
>  {
>  int i = 0, d = 0;
>  int ret;
> @@ -167,10 +169,24 @@ int xbzrle_decode_buffer(uint8_t *src, int slen, 
> uint8_t *dst, int dlen)
>  return -1;
>  }
>  
> -memcpy(dst + d, src + i, count);
> +if (!is_pmem) {
> +memcpy(dst + d, src + i, count);
> +} else {
> +pmem_memcpy_nodrain(dst + d, src + i, count);
> +}
>  d += count;
>  i += count;
>  }
>  
>  return d;
>  }
> +
> +int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t *dst, int dlen)
> +{
> +return xbzrle_decode_buffer_common(src, slen, dst, dlen, false);
> +}
> +
> +int xbzrle_decode_buffer_to_pmem(uint8_t *src, int slen, uint8_t *dst, int 
> dlen)
> +{
> +return xbzrle_decode_buffer_common(src, slen, dst, dlen, true);
> +}

Again I wouldn't bother with these two wrappers, just add the is_pmem
flag or pointer to xbzrle_decode_buffer.  Note that in xbzrle load this
is doing lots of small copies; I'm not sure if this changes your needs.

Dave

> diff --git a/migration/xbzrle.h b/migration/xbzrle.h
> index a0db507b9c..ac5ae32666 100644
> --- a/migration/xbzrle.h
> +++ b/migration/xbzrle.h
> @@ -18,4 +18,5 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t 
> *new_buf, int slen,
>   uint8_t *dst, int dlen);
>  
>  int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t *dst, int dlen);
> +int xbzrle_decode_buffer_to_pmem(uint8_t *src, int slen, uint8_t *dst, int 
> dlen);
>  #endif
> -- 
> 2.14.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v2 8/8] migration/ram: ensure write persistence on loading xbzrle pages to PMEM

[Haozhong Zhang]

When loading a xbzrle encoded page to persistent memory, load the data
via libpmem function pmem_memcpy_nodrain() instead of memcpy().
Combined with a call to pmem_drain() at the end of memory loading, we
can guarantee those xbzrle encoded pages are persistently loaded to PMEM.

Signed-off-by: Haozhong Zhang 
---
 migration/ram.c| 15 ++-
 migration/xbzrle.c | 20 ++--
 migration/xbzrle.h |  1 +
 3 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 924d2b9537..87f977617d 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2388,10 +2388,10 @@ static void ram_save_pending(QEMUFile *f, void *opaque, 
uint64_t max_size,
 }
 }
 
-static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
+static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host, bool is_pmem)
 {
 unsigned int xh_len;
-int xh_flags;
+int xh_flags, rc;
 uint8_t *loaded_data;
 
 /* extract RLE header */
@@ -2413,8 +2413,13 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, 
void *host)
 qemu_get_buffer_in_place(f, _data, xh_len);
 
 /* decode RLE */
-if (xbzrle_decode_buffer(loaded_data, xh_len, host,
- TARGET_PAGE_SIZE) == -1) {
+if (!is_pmem) {
+rc = xbzrle_decode_buffer(loaded_data, xh_len, host, TARGET_PAGE_SIZE);
+} else {
+rc = xbzrle_decode_buffer_to_pmem(loaded_data, xh_len, host,
+  TARGET_PAGE_SIZE);
+}
+if (rc == -1) {
 error_report("Failed to load XBZRLE page - decode error!");
 return -1;
 }
@@ -2974,7 +2979,7 @@ static int ram_load(QEMUFile *f, void *opaque, int 
version_id)
 break;
 
 case RAM_SAVE_FLAG_XBZRLE:
-if (load_xbzrle(f, addr, host) < 0) {
+if (load_xbzrle(f, addr, host, is_pmem) < 0) {
 error_report("Failed to decompress XBZRLE page at "
  RAM_ADDR_FMT, addr);
 ret = -EINVAL;
diff --git a/migration/xbzrle.c b/migration/xbzrle.c
index 1ba482ded9..499d8e1bfb 100644
--- a/migration/xbzrle.c
+++ b/migration/xbzrle.c
@@ -12,6 +12,7 @@
  */
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
+#include "qemu/pmem.h"
 #include "xbzrle.h"
 
 /*
@@ -126,7 +127,8 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t 
*new_buf, int slen,
 return d;
 }
 
-int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t *dst, int dlen)
+static int xbzrle_decode_buffer_common(uint8_t *src, int slen, uint8_t *dst,
+   int dlen, bool is_pmem)
 {
 int i = 0, d = 0;
 int ret;
@@ -167,10 +169,24 @@ int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t 
*dst, int dlen)
 return -1;
 }
 
-memcpy(dst + d, src + i, count);
+if (!is_pmem) {
+memcpy(dst + d, src + i, count);
+} else {
+pmem_memcpy_nodrain(dst + d, src + i, count);
+}
 d += count;
 i += count;
 }
 
 return d;
 }
+
+int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t *dst, int dlen)
+{
+return xbzrle_decode_buffer_common(src, slen, dst, dlen, false);
+}
+
+int xbzrle_decode_buffer_to_pmem(uint8_t *src, int slen, uint8_t *dst, int 
dlen)
+{
+return xbzrle_decode_buffer_common(src, slen, dst, dlen, true);
+}
diff --git a/migration/xbzrle.h b/migration/xbzrle.h
index a0db507b9c..ac5ae32666 100644
--- a/migration/xbzrle.h
+++ b/migration/xbzrle.h
@@ -18,4 +18,5 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, 
int slen,
  uint8_t *dst, int dlen);
 
 int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t *dst, int dlen);
+int xbzrle_decode_buffer_to_pmem(uint8_t *src, int slen, uint8_t *dst, int 
dlen);
 #endif
-- 
2.14.1