diff mbox series

[3/4] migration: ram: Switch to ram block writeback

Message ID 20190910095610.4546-4-beata.michalska@linaro.org (mailing list archive)
State New, archived
Headers show
Series target/arm: Support for Data Cache Clean up to PoP | expand

Commit Message

Beata Michalska Sept. 10, 2019, 9:56 a.m. UTC
Switch to ram block writeback for pmem migration.

Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
---
 migration/ram.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

Dr. David Alan Gilbert Sept. 10, 2019, 10:26 a.m. UTC | #1
* Beata Michalska (beata.michalska@linaro.org) wrote:
> Switch to ram block writeback for pmem migration.
> 
> Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
> ---
>  migration/ram.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index b01a37e7ca..8ea0bd63fc 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -33,7 +33,6 @@
>  #include "qemu/bitops.h"
>  #include "qemu/bitmap.h"
>  #include "qemu/main-loop.h"
> -#include "qemu/pmem.h"
>  #include "xbzrle.h"
>  #include "ram.h"
>  #include "migration.h"
> @@ -4064,9 +4063,7 @@ static int ram_load_cleanup(void *opaque)
>      RAMBlock *rb;
>  
>      RAMBLOCK_FOREACH_NOT_IGNORED(rb) {
> -        if (ramblock_is_pmem(rb)) {
> -            pmem_persist(rb->host, rb->used_length);
> -        }
> +        qemu_ram_block_writeback(rb);

ACK for migration

Although I do worry that if you really have pmem hardware, is it better
to fail the migration if you don't have libpmem available?

Dave

>      }
>  
>      xbzrle_load_cleanup();
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Beata Michalska Sept. 10, 2019, 11:28 a.m. UTC | #2
On Tue, 10 Sep 2019 at 12:26, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Beata Michalska (beata.michalska@linaro.org) wrote:
> > Switch to ram block writeback for pmem migration.
> >
> > Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
> > ---
> >  migration/ram.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/migration/ram.c b/migration/ram.c
> > index b01a37e7ca..8ea0bd63fc 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -33,7 +33,6 @@
> >  #include "qemu/bitops.h"
> >  #include "qemu/bitmap.h"
> >  #include "qemu/main-loop.h"
> > -#include "qemu/pmem.h"
> >  #include "xbzrle.h"
> >  #include "ram.h"
> >  #include "migration.h"
> > @@ -4064,9 +4063,7 @@ static int ram_load_cleanup(void *opaque)
> >      RAMBlock *rb;
> >
> >      RAMBLOCK_FOREACH_NOT_IGNORED(rb) {
> > -        if (ramblock_is_pmem(rb)) {
> > -            pmem_persist(rb->host, rb->used_length);
> > -        }
> > +        qemu_ram_block_writeback(rb);
>
> ACK for migration
>
> Although I do worry that if you really have pmem hardware, is it better
> to fail the migration if you don't have libpmem available?

According to the PMDG man page, pmem_persist is supposed to be
equivalent for the msync.
It's just more performant. So in case of real pmem hardware it should
be all good.

[http://pmem.io/pmdk/manpages/linux/v1.2/libpmem.3.html]

BR
Beata
>
> Dave
>
> >      }
> >
> >      xbzrle_load_cleanup();
> > --
> > 2.17.1
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert Sept. 10, 2019, 1:16 p.m. UTC | #3
* Beata Michalska (beata.michalska@linaro.org) wrote:
> On Tue, 10 Sep 2019 at 12:26, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Beata Michalska (beata.michalska@linaro.org) wrote:
> > > Switch to ram block writeback for pmem migration.
> > >
> > > Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
> > > ---
> > >  migration/ram.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index b01a37e7ca..8ea0bd63fc 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -33,7 +33,6 @@
> > >  #include "qemu/bitops.h"
> > >  #include "qemu/bitmap.h"
> > >  #include "qemu/main-loop.h"
> > > -#include "qemu/pmem.h"
> > >  #include "xbzrle.h"
> > >  #include "ram.h"
> > >  #include "migration.h"
> > > @@ -4064,9 +4063,7 @@ static int ram_load_cleanup(void *opaque)
> > >      RAMBlock *rb;
> > >
> > >      RAMBLOCK_FOREACH_NOT_IGNORED(rb) {
> > > -        if (ramblock_is_pmem(rb)) {
> > > -            pmem_persist(rb->host, rb->used_length);
> > > -        }
> > > +        qemu_ram_block_writeback(rb);
> >
> > ACK for migration
> >
> > Although I do worry that if you really have pmem hardware, is it better
> > to fail the migration if you don't have libpmem available?
> 
> According to the PMDG man page, pmem_persist is supposed to be
> equivalent for the msync.

OK, but you do define qemu_ram_block_writeback to fall back to fdatasync;
so that would be too little?

> It's just more performant. So in case of real pmem hardware it should
> be all good.
> 
> [http://pmem.io/pmdk/manpages/linux/v1.2/libpmem.3.html]

Dave

> 
> BR
> Beata
> >
> > Dave
> >
> > >      }
> > >
> > >      xbzrle_load_cleanup();
> > > --
> > > 2.17.1
> > >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Beata Michalska Sept. 10, 2019, 2:21 p.m. UTC | #4
On Tue, 10 Sep 2019 at 14:16, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Beata Michalska (beata.michalska@linaro.org) wrote:
> > On Tue, 10 Sep 2019 at 12:26, Dr. David Alan Gilbert
> > <dgilbert@redhat.com> wrote:
> > >
> > > * Beata Michalska (beata.michalska@linaro.org) wrote:
> > > > Switch to ram block writeback for pmem migration.
> > > >
> > > > Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
> > > > ---
> > > >  migration/ram.c | 5 +----
> > > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > >
> > > > diff --git a/migration/ram.c b/migration/ram.c
> > > > index b01a37e7ca..8ea0bd63fc 100644
> > > > --- a/migration/ram.c
> > > > +++ b/migration/ram.c
> > > > @@ -33,7 +33,6 @@
> > > >  #include "qemu/bitops.h"
> > > >  #include "qemu/bitmap.h"
> > > >  #include "qemu/main-loop.h"
> > > > -#include "qemu/pmem.h"
> > > >  #include "xbzrle.h"
> > > >  #include "ram.h"
> > > >  #include "migration.h"
> > > > @@ -4064,9 +4063,7 @@ static int ram_load_cleanup(void *opaque)
> > > >      RAMBlock *rb;
> > > >
> > > >      RAMBLOCK_FOREACH_NOT_IGNORED(rb) {
> > > > -        if (ramblock_is_pmem(rb)) {
> > > > -            pmem_persist(rb->host, rb->used_length);
> > > > -        }
> > > > +        qemu_ram_block_writeback(rb);
> > >
> > > ACK for migration
> > >
> > > Although I do worry that if you really have pmem hardware, is it better
> > > to fail the migration if you don't have libpmem available?
> >
> > According to the PMDG man page, pmem_persist is supposed to be
> > equivalent for the msync.
>
> OK, but you do define qemu_ram_block_writeback to fall back to fdatasync;
> so that would be too little?

Actually it shouldn't. All will end-up in vfs_fsync_range; msync will
narrow the range.
fdatasync will trigger the same call just that with a wider range. At
least for Linux.
fdatasync will also fallback to fsync if it is not available.
So it's going from the best case scenario (as of performance and range of mem
to be synced) towards the worst case one.

I should probably double-check earlier versions of Linux.
I'll also try to verify that for other host variants.

BTW: Thank you for having a look at the changes.

BR
Beata

>
> > It's just more performant. So in case of real pmem hardware it should
> > be all good.
> >
> > [http://pmem.io/pmdk/manpages/linux/v1.2/libpmem.3.html]
>
> Dave
>
> >
> > BR
> > Beata
> > >
> > > Dave
> > >
> > > >      }
> > > >
> > > >      xbzrle_load_cleanup();
> > > > --
> > > > 2.17.1
> > > >
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert Sept. 11, 2019, 10:36 a.m. UTC | #5
* Beata Michalska (beata.michalska@linaro.org) wrote:
> On Tue, 10 Sep 2019 at 14:16, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Beata Michalska (beata.michalska@linaro.org) wrote:
> > > On Tue, 10 Sep 2019 at 12:26, Dr. David Alan Gilbert
> > > <dgilbert@redhat.com> wrote:
> > > >
> > > > * Beata Michalska (beata.michalska@linaro.org) wrote:
> > > > > Switch to ram block writeback for pmem migration.
> > > > >
> > > > > Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
> > > > > ---
> > > > >  migration/ram.c | 5 +----
> > > > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/migration/ram.c b/migration/ram.c
> > > > > index b01a37e7ca..8ea0bd63fc 100644
> > > > > --- a/migration/ram.c
> > > > > +++ b/migration/ram.c
> > > > > @@ -33,7 +33,6 @@
> > > > >  #include "qemu/bitops.h"
> > > > >  #include "qemu/bitmap.h"
> > > > >  #include "qemu/main-loop.h"
> > > > > -#include "qemu/pmem.h"
> > > > >  #include "xbzrle.h"
> > > > >  #include "ram.h"
> > > > >  #include "migration.h"
> > > > > @@ -4064,9 +4063,7 @@ static int ram_load_cleanup(void *opaque)
> > > > >      RAMBlock *rb;
> > > > >
> > > > >      RAMBLOCK_FOREACH_NOT_IGNORED(rb) {
> > > > > -        if (ramblock_is_pmem(rb)) {
> > > > > -            pmem_persist(rb->host, rb->used_length);
> > > > > -        }
> > > > > +        qemu_ram_block_writeback(rb);
> > > >
> > > > ACK for migration
> > > >
> > > > Although I do worry that if you really have pmem hardware, is it better
> > > > to fail the migration if you don't have libpmem available?
> > >
> > > According to the PMDG man page, pmem_persist is supposed to be
> > > equivalent for the msync.
> >
> > OK, but you do define qemu_ram_block_writeback to fall back to fdatasync;
> > so that would be too little?
> 
> Actually it shouldn't. All will end-up in vfs_fsync_range; msync will
> narrow the range.
> fdatasync will trigger the same call just that with a wider range. At
> least for Linux.
> fdatasync will also fallback to fsync if it is not available.
> So it's going from the best case scenario (as of performance and range of mem
> to be synced) towards the worst case one.
> 
> I should probably double-check earlier versions of Linux.
> I'll also try to verify that for other host variants.

Well I guess it should probably follow whatever Posix says;  it's OK to
make Linux specific assumptions for Linux specific bits - but you can't
do it by code examination to guarantee it'll be right for other
platforms, especially if this is in code ifdef'd for portability.
Also it needs commenting to explain why it's safe to avoid someone else
asking this question.

> BTW: Thank you for having a look at the changes.

No problem.

Dave

> BR
> Beata
> 
> >
> > > It's just more performant. So in case of real pmem hardware it should
> > > be all good.
> > >
> > > [http://pmem.io/pmdk/manpages/linux/v1.2/libpmem.3.html]
> >
> > Dave
> >
> > >
> > > BR
> > > Beata
> > > >
> > > > Dave
> > > >
> > > > >      }
> > > > >
> > > > >      xbzrle_load_cleanup();
> > > > > --
> > > > > 2.17.1
> > > > >
> > > > --
> > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Beata Michalska Sept. 12, 2019, 9:10 a.m. UTC | #6
On Wed, 11 Sep 2019 at 11:36, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Beata Michalska (beata.michalska@linaro.org) wrote:
> > On Tue, 10 Sep 2019 at 14:16, Dr. David Alan Gilbert
> > <dgilbert@redhat.com> wrote:
> > >
> > > * Beata Michalska (beata.michalska@linaro.org) wrote:
> > > > On Tue, 10 Sep 2019 at 12:26, Dr. David Alan Gilbert
> > > > <dgilbert@redhat.com> wrote:
> > > > >
> > > > > * Beata Michalska (beata.michalska@linaro.org) wrote:
> > > > > > Switch to ram block writeback for pmem migration.
> > > > > >
> > > > > > Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
> > > > > > ---
> > > > > >  migration/ram.c | 5 +----
> > > > > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/migration/ram.c b/migration/ram.c
> > > > > > index b01a37e7ca..8ea0bd63fc 100644
> > > > > > --- a/migration/ram.c
> > > > > > +++ b/migration/ram.c
> > > > > > @@ -33,7 +33,6 @@
> > > > > >  #include "qemu/bitops.h"
> > > > > >  #include "qemu/bitmap.h"
> > > > > >  #include "qemu/main-loop.h"
> > > > > > -#include "qemu/pmem.h"
> > > > > >  #include "xbzrle.h"
> > > > > >  #include "ram.h"
> > > > > >  #include "migration.h"
> > > > > > @@ -4064,9 +4063,7 @@ static int ram_load_cleanup(void *opaque)
> > > > > >      RAMBlock *rb;
> > > > > >
> > > > > >      RAMBLOCK_FOREACH_NOT_IGNORED(rb) {
> > > > > > -        if (ramblock_is_pmem(rb)) {
> > > > > > -            pmem_persist(rb->host, rb->used_length);
> > > > > > -        }
> > > > > > +        qemu_ram_block_writeback(rb);
> > > > >
> > > > > ACK for migration
> > > > >
> > > > > Although I do worry that if you really have pmem hardware, is it better
> > > > > to fail the migration if you don't have libpmem available?
> > > >
> > > > According to the PMDG man page, pmem_persist is supposed to be
> > > > equivalent for the msync.
> > >
> > > OK, but you do define qemu_ram_block_writeback to fall back to fdatasync;
> > > so that would be too little?
> >
> > Actually it shouldn't. All will end-up in vfs_fsync_range; msync will
> > narrow the range.
> > fdatasync will trigger the same call just that with a wider range. At
> > least for Linux.
> > fdatasync will also fallback to fsync if it is not available.
> > So it's going from the best case scenario (as of performance and range of mem
> > to be synced) towards the worst case one.
> >
> > I should probably double-check earlier versions of Linux.
> > I'll also try to verify that for other host variants.
>
> Well I guess it should probably follow whatever Posix says;  it's OK to
> make Linux specific assumptions for Linux specific bits - but you can't
> do it by code examination to guarantee it'll be right for other
> platforms, especially if this is in code ifdef'd for portability.
> Also it needs commenting to explain why it's safe to avoid someone else
> asking this question.
>
I will definitely address that in the next version.
Will just wait a bit to potentially gather more input
on the series.

> > BTW: Thank you for having a look at the changes.
>
> No problem.
>
Thanks again.

BR
Beata
> Dave
>
> > BR
> > Beata
> >
> > >
> > > > It's just more performant. So in case of real pmem hardware it should
> > > > be all good.
> > > >
> > > > [http://pmem.io/pmdk/manpages/linux/v1.2/libpmem.3.html]
> > >
> > > Dave
> > >
> > > >
> > > > BR
> > > > Beata
> > > > >
> > > > > Dave
> > > > >
> > > > > >      }
> > > > > >
> > > > > >      xbzrle_load_cleanup();
> > > > > > --
> > > > > > 2.17.1
> > > > > >
> > > > > --
> > > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Richard Henderson Sept. 24, 2019, 4:30 p.m. UTC | #7
On 9/10/19 2:56 AM, Beata Michalska wrote:
> Switch to ram block writeback for pmem migration.
> 
> Signed-off-by: Beata Michalska <beata.michalska@linaro.org>
> ---
>  migration/ram.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
diff mbox series

Patch

diff --git a/migration/ram.c b/migration/ram.c
index b01a37e7ca..8ea0bd63fc 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -33,7 +33,6 @@ 
 #include "qemu/bitops.h"
 #include "qemu/bitmap.h"
 #include "qemu/main-loop.h"
-#include "qemu/pmem.h"
 #include "xbzrle.h"
 #include "ram.h"
 #include "migration.h"
@@ -4064,9 +4063,7 @@  static int ram_load_cleanup(void *opaque)
     RAMBlock *rb;
 
     RAMBLOCK_FOREACH_NOT_IGNORED(rb) {
-        if (ramblock_is_pmem(rb)) {
-            pmem_persist(rb->host, rb->used_length);
-        }
+        qemu_ram_block_writeback(rb);
     }
 
     xbzrle_load_cleanup();