On 02/07/18 11:49 +0000, 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 <haozhong.zh...@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 / dgilb...@redhat.com / Manchester, UK