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

2018-02-07 Thread Haozhong Zhang 
On 02/07/18 11:49 +, Dr. David Alan Gilbert wrote:
> * Haozhong Zhang (haozhong.zh...@intel.com) wrote:
> > When loading a normal page to persistent memory, load its data by
> > libpmem function pmem_memcpy_nodrain() instead of memcpy(). Combined
> > with a call to pmem_drain() at the end of memory loading, we can
> > guarantee all those normal pages are persistenly loaded to PMEM.
> > 
> > Signed-off-by: Haozhong Zhang 
> > ---
> >  include/migration/qemu-file-types.h |  1 +
> >  include/qemu/pmem.h |  6 ++
> >  migration/qemu-file.c   | 41 
> > -
> >  migration/ram.c |  6 +-
> >  4 files changed, 43 insertions(+), 11 deletions(-)
> > 
> > diff --git a/include/migration/qemu-file-types.h 
> > b/include/migration/qemu-file-types.h
> > index bd6d7dd7f9..bb5c547498 100644
> > --- a/include/migration/qemu-file-types.h
> > +++ b/include/migration/qemu-file-types.h
> > @@ -34,6 +34,7 @@ void qemu_put_be16(QEMUFile *f, unsigned int v);
> >  void qemu_put_be32(QEMUFile *f, unsigned int v);
> >  void qemu_put_be64(QEMUFile *f, uint64_t v);
> >  size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size);
> > +size_t qemu_get_buffer_to_pmem(QEMUFile *f, uint8_t *buf, size_t size);
> >  
> >  int qemu_get_byte(QEMUFile *f);
> >  
> > diff --git a/include/qemu/pmem.h b/include/qemu/pmem.h
> > index 861d8ecc21..77ee1fc4eb 100644
> > --- a/include/qemu/pmem.h
> > +++ b/include/qemu/pmem.h
> > @@ -26,6 +26,12 @@ pmem_memcpy_persist(void *pmemdest, const void *src, 
> > size_t len)
> >  return memcpy(pmemdest, src, len);
> >  }
> >  
> > +static inline void *
> > +pmem_memcpy_nodrain(void *pmemdest, const void *src, size_t len)
> > +{
> > +return memcpy(pmemdest, src, len);
> > +}
> > +
> >  static inline void *pmem_memset_nodrain(void *pmemdest, int c, size_t len)
> >  {
> >  return memset(pmemdest, c, len);
> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> > index 2ab2bf362d..7e573010d9 100644
> > --- a/migration/qemu-file.c
> > +++ b/migration/qemu-file.c
> > @@ -26,6 +26,7 @@
> >  #include "qemu-common.h"
> >  #include "qemu/error-report.h"
> >  #include "qemu/iov.h"
> > +#include "qemu/pmem.h"
> >  #include "migration.h"
> >  #include "qemu-file.h"
> >  #include "trace.h"
> > @@ -471,15 +472,8 @@ size_t qemu_peek_buffer(QEMUFile *f, uint8_t **buf, 
> > size_t size, size_t offset)
> >  return size;
> >  }
> >  
> > -/*
> > - * Read 'size' bytes of data from the file into buf.
> > - * 'size' can be larger than the internal buffer.
> > - *
> > - * It will return size bytes unless there was an error, in which case it 
> > will
> > - * return as many as it managed to read (assuming blocking fd's which
> > - * all current QEMUFile are)
> > - */
> > -size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size)
> > +static size_t
> > +qemu_get_buffer_common(QEMUFile *f, uint8_t *buf, size_t size, bool 
> > is_pmem)
> >  {
> >  size_t pending = size;
> >  size_t done = 0;
> > @@ -492,7 +486,11 @@ size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, 
> > size_t size)
> >  if (res == 0) {
> >  return done;
> >  }
> > -memcpy(buf, src, res);
> > +if (!is_pmem) {
> > +memcpy(buf, src, res);
> > +} else {
> > +pmem_memcpy_nodrain(buf, src, res);
> > +}
> 
> I see why you're doing it, but again I'm surprised it's ended up having
> to modify qemu-file.

Well, the current programming model of pmem requires users to take
special care on the data persistence, so QEMU (as a user of pmem) has
to do that as well.

> 
> >  qemu_file_skip(f, res);
> >  buf += res;
> >  pending -= res;
> > @@ -501,6 +499,29 @@ size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, 
> > size_t size)
> >  return done;
> >  }
> >  
> > +/*
> > + * Read 'size' bytes of data from the file into buf.
> > + * 'size' can be larger than the internal buffer.
> > + *
> > + * It will return size bytes unless there was an error, in which case it 
> > will
> > + * return as many as it managed to read (assuming blocking fd's which
> > + * all current QEMUFile are)
> > + */
> > +size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size)
> > +{
> > +return qemu_get_buffer_common(f, buf, size, false);
> > +}
> > +
> > +/*
> > + * Mostly the same as qemu_get_buffer(), except that
> > + * 1) it's for the case that 'buf' is in the persistent memory, and
> > + * 2) it takes necessary operations to ensure the data persistence in 
> > 'buf'.
> > + */
> > +size_t qemu_get_buffer_to_pmem(QEMUFile *f, uint8_t *buf, size_t size)
> > +{
> > +return qemu_get_buffer_common(f, buf, size, true);
> > +}
> > +
> >  /*
> >   * Read 'size' bytes of data from the file.
> >   * 'size' can be larger than the internal buffer.
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 5a0e503818..5a79bbff64 100644
> >

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

2018-02-07 Thread Dr. David Alan Gilbert 
* Haozhong Zhang (haozhong.zh...@intel.com) wrote:
> When loading a normal page to persistent memory, load its data by
> libpmem function pmem_memcpy_nodrain() instead of memcpy(). Combined
> with a call to pmem_drain() at the end of memory loading, we can
> guarantee all those normal pages are persistenly loaded to PMEM.
> 
> Signed-off-by: Haozhong Zhang 
> ---
>  include/migration/qemu-file-types.h |  1 +
>  include/qemu/pmem.h |  6 ++
>  migration/qemu-file.c   | 41 
> -
>  migration/ram.c |  6 +-
>  4 files changed, 43 insertions(+), 11 deletions(-)
> 
> diff --git a/include/migration/qemu-file-types.h 
> b/include/migration/qemu-file-types.h
> index bd6d7dd7f9..bb5c547498 100644
> --- a/include/migration/qemu-file-types.h
> +++ b/include/migration/qemu-file-types.h
> @@ -34,6 +34,7 @@ void qemu_put_be16(QEMUFile *f, unsigned int v);
>  void qemu_put_be32(QEMUFile *f, unsigned int v);
>  void qemu_put_be64(QEMUFile *f, uint64_t v);
>  size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size);
> +size_t qemu_get_buffer_to_pmem(QEMUFile *f, uint8_t *buf, size_t size);
>  
>  int qemu_get_byte(QEMUFile *f);
>  
> diff --git a/include/qemu/pmem.h b/include/qemu/pmem.h
> index 861d8ecc21..77ee1fc4eb 100644
> --- a/include/qemu/pmem.h
> +++ b/include/qemu/pmem.h
> @@ -26,6 +26,12 @@ pmem_memcpy_persist(void *pmemdest, const void *src, 
> size_t len)
>  return memcpy(pmemdest, src, len);
>  }
>  
> +static inline void *
> +pmem_memcpy_nodrain(void *pmemdest, const void *src, size_t len)
> +{
> +return memcpy(pmemdest, src, len);
> +}
> +
>  static inline void *pmem_memset_nodrain(void *pmemdest, int c, size_t len)
>  {
>  return memset(pmemdest, c, len);
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 2ab2bf362d..7e573010d9 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -26,6 +26,7 @@
>  #include "qemu-common.h"
>  #include "qemu/error-report.h"
>  #include "qemu/iov.h"
> +#include "qemu/pmem.h"
>  #include "migration.h"
>  #include "qemu-file.h"
>  #include "trace.h"
> @@ -471,15 +472,8 @@ size_t qemu_peek_buffer(QEMUFile *f, uint8_t **buf, 
> size_t size, size_t offset)
>  return size;
>  }
>  
> -/*
> - * Read 'size' bytes of data from the file into buf.
> - * 'size' can be larger than the internal buffer.
> - *
> - * It will return size bytes unless there was an error, in which case it will
> - * return as many as it managed to read (assuming blocking fd's which
> - * all current QEMUFile are)
> - */
> -size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size)
> +static size_t
> +qemu_get_buffer_common(QEMUFile *f, uint8_t *buf, size_t size, bool is_pmem)
>  {
>  size_t pending = size;
>  size_t done = 0;
> @@ -492,7 +486,11 @@ size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t 
> size)
>  if (res == 0) {
>  return done;
>  }
> -memcpy(buf, src, res);
> +if (!is_pmem) {
> +memcpy(buf, src, res);
> +} else {
> +pmem_memcpy_nodrain(buf, src, res);
> +}

I see why you're doing it, but again I'm surprised it's ended up having
to modify qemu-file.

>  qemu_file_skip(f, res);
>  buf += res;
>  pending -= res;
> @@ -501,6 +499,29 @@ size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t 
> size)
>  return done;
>  }
>  
> +/*
> + * Read 'size' bytes of data from the file into buf.
> + * 'size' can be larger than the internal buffer.
> + *
> + * It will return size bytes unless there was an error, in which case it will
> + * return as many as it managed to read (assuming blocking fd's which
> + * all current QEMUFile are)
> + */
> +size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size)
> +{
> +return qemu_get_buffer_common(f, buf, size, false);
> +}
> +
> +/*
> + * Mostly the same as qemu_get_buffer(), except that
> + * 1) it's for the case that 'buf' is in the persistent memory, and
> + * 2) it takes necessary operations to ensure the data persistence in 'buf'.
> + */
> +size_t qemu_get_buffer_to_pmem(QEMUFile *f, uint8_t *buf, size_t size)
> +{
> +return qemu_get_buffer_common(f, buf, size, true);
> +}
> +
>  /*
>   * Read 'size' bytes of data from the file.
>   * 'size' can be larger than the internal buffer.
> diff --git a/migration/ram.c b/migration/ram.c
> index 5a0e503818..5a79bbff64 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2950,7 +2950,11 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>  break;
>  
>  case RAM_SAVE_FLAG_PAGE:
> -qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> +if (!is_pmem) {
> +qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> +} else {
> +qemu_get_buffer_to_pmem(f, host, TARGET_PAGE_SIZE);
> +}

can you just expose

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

2018-02-06 Thread Haozhong Zhang 
When loading a normal page to persistent memory, load its data by
libpmem function pmem_memcpy_nodrain() instead of memcpy(). Combined
with a call to pmem_drain() at the end of memory loading, we can
guarantee all those normal pages are persistenly loaded to PMEM.

Signed-off-by: Haozhong Zhang 
---
 include/migration/qemu-file-types.h |  1 +
 include/qemu/pmem.h |  6 ++
 migration/qemu-file.c   | 41 -
 migration/ram.c |  6 +-
 4 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/include/migration/qemu-file-types.h 
b/include/migration/qemu-file-types.h
index bd6d7dd7f9..bb5c547498 100644
--- a/include/migration/qemu-file-types.h
+++ b/include/migration/qemu-file-types.h
@@ -34,6 +34,7 @@ void qemu_put_be16(QEMUFile *f, unsigned int v);
 void qemu_put_be32(QEMUFile *f, unsigned int v);
 void qemu_put_be64(QEMUFile *f, uint64_t v);
 size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size);
+size_t qemu_get_buffer_to_pmem(QEMUFile *f, uint8_t *buf, size_t size);
 
 int qemu_get_byte(QEMUFile *f);
 
diff --git a/include/qemu/pmem.h b/include/qemu/pmem.h
index 861d8ecc21..77ee1fc4eb 100644
--- a/include/qemu/pmem.h
+++ b/include/qemu/pmem.h
@@ -26,6 +26,12 @@ pmem_memcpy_persist(void *pmemdest, const void *src, size_t 
len)
 return memcpy(pmemdest, src, len);
 }
 
+static inline void *
+pmem_memcpy_nodrain(void *pmemdest, const void *src, size_t len)
+{
+return memcpy(pmemdest, src, len);
+}
+
 static inline void *pmem_memset_nodrain(void *pmemdest, int c, size_t len)
 {
 return memset(pmemdest, c, len);
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 2ab2bf362d..7e573010d9 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -26,6 +26,7 @@
 #include "qemu-common.h"
 #include "qemu/error-report.h"
 #include "qemu/iov.h"
+#include "qemu/pmem.h"
 #include "migration.h"
 #include "qemu-file.h"
 #include "trace.h"
@@ -471,15 +472,8 @@ size_t qemu_peek_buffer(QEMUFile *f, uint8_t **buf, size_t 
size, size_t offset)
 return size;
 }
 
-/*
- * Read 'size' bytes of data from the file into buf.
- * 'size' can be larger than the internal buffer.
- *
- * It will return size bytes unless there was an error, in which case it will
- * return as many as it managed to read (assuming blocking fd's which
- * all current QEMUFile are)
- */
-size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size)
+static size_t
+qemu_get_buffer_common(QEMUFile *f, uint8_t *buf, size_t size, bool is_pmem)
 {
 size_t pending = size;
 size_t done = 0;
@@ -492,7 +486,11 @@ size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t 
size)
 if (res == 0) {
 return done;
 }
-memcpy(buf, src, res);
+if (!is_pmem) {
+memcpy(buf, src, res);
+} else {
+pmem_memcpy_nodrain(buf, src, res);
+}
 qemu_file_skip(f, res);
 buf += res;
 pending -= res;
@@ -501,6 +499,29 @@ size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t 
size)
 return done;
 }
 
+/*
+ * Read 'size' bytes of data from the file into buf.
+ * 'size' can be larger than the internal buffer.
+ *
+ * It will return size bytes unless there was an error, in which case it will
+ * return as many as it managed to read (assuming blocking fd's which
+ * all current QEMUFile are)
+ */
+size_t qemu_get_buffer(QEMUFile *f, uint8_t *buf, size_t size)
+{
+return qemu_get_buffer_common(f, buf, size, false);
+}
+
+/*
+ * Mostly the same as qemu_get_buffer(), except that
+ * 1) it's for the case that 'buf' is in the persistent memory, and
+ * 2) it takes necessary operations to ensure the data persistence in 'buf'.
+ */
+size_t qemu_get_buffer_to_pmem(QEMUFile *f, uint8_t *buf, size_t size)
+{
+return qemu_get_buffer_common(f, buf, size, true);
+}
+
 /*
  * Read 'size' bytes of data from the file.
  * 'size' can be larger than the internal buffer.
diff --git a/migration/ram.c b/migration/ram.c
index 5a0e503818..5a79bbff64 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2950,7 +2950,11 @@ static int ram_load(QEMUFile *f, void *opaque, int 
version_id)
 break;
 
 case RAM_SAVE_FLAG_PAGE:
-qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
+if (!is_pmem) {
+qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
+} else {
+qemu_get_buffer_to_pmem(f, host, TARGET_PAGE_SIZE);
+}
 break;
 
 case RAM_SAVE_FLAG_COMPRESS_PAGE:
-- 
2.14.1