| Message ID | 20180207073331.14158-7-haozhong.zhang@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | nvdimm: guarantee persistence of QEMU writes to persistent memory | expand |
* Haozhong Zhang (haozhong.zhang@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 <haozhong.zhang@intel.com> > --- > 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_get_buffer_common instead of making it static and just pass in the is_pmem flag. Dave > break; > > case RAM_SAVE_FLAG_COMPRESS_PAGE: > -- > 2.14.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 02/07/18 11:49 +0000, Dr. David Alan Gilbert wrote: > * Haozhong Zhang (haozhong.zhang@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 <haozhong.zhang@intel.com> > > --- > > 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 > > --- 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_get_buffer_common instead of making it static > and just pass in the is_pmem flag. Or perhaps passing a memcpyfunc function similarly to your comment on patch 5. Thanks, Haozhong > > Dave > > > break; > > > > case RAM_SAVE_FLAG_COMPRESS_PAGE: > > -- > > 2.14.1 > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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:
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 <haozhong.zhang@intel.com> --- 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(-)