Re: [Qemu-devel] [PATCH V7 RESEND 12/17] savevm: split the process of different stages for loadvm/savevm

2018-06-21 Thread Zhang Chen 
On Wed, Jun 20, 2018 at 3:00 AM, Dr. David Alan Gilbert  wrote:

> * Zhang Chen (zhangc...@gmail.com) wrote:
> > On Wed, May 16, 2018 at 2:56 AM, Dr. David Alan Gilbert <
> dgilb...@redhat.com
> > > wrote:
> >
> > > * Zhang Chen (zhangc...@gmail.com) wrote:
> > > > From: zhanghailiang 
> > > >
> > > > There are several stages during loadvm/savevm process. In different
> > > stage,
> > > > migration incoming processes different types of sections.
> > > > We want to control these stages more accuracy, it will benefit COLO
> > > > performance, we don't have to save type of QEMU_VM_SECTION_START
> > > > sections everytime while do checkpoint, besides, we want to separate
> > > > the process of saving/loading memory and devices state.
> > > >
> > > > So we add three new helper functions: qemu_load_device_state() and
> > > > qemu_savevm_live_state() to achieve different process during
> migration.
> > > >
> > > > Besides, we make qemu_loadvm_state_main() and
> qemu_save_device_state()
> > > > public, and simplify the codes of qemu_save_device_state() by
> calling the
> > > > wrapper qemu_savevm_state_header().
> > > >
> > > > Signed-off-by: zhanghailiang 
> > > > Signed-off-by: Li Zhijian 
> > > > Signed-off-by: Zhang Chen 
> > > > Reviewed-by: Dr. David Alan Gilbert 
> > > > ---
> > > >  migration/colo.c   | 36 
> > > >  migration/savevm.c | 35 ---
> > > >  migration/savevm.h |  4 
> > > >  3 files changed, 60 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/migration/colo.c b/migration/colo.c
> > > > index cdff0a2490..5b055f79f1 100644
> > > > --- a/migration/colo.c
> > > > +++ b/migration/colo.c
> > > > @@ -30,6 +30,7 @@
> > > >  #include "block/block.h"
> > > >  #include "qapi/qapi-events-migration.h"
> > > >  #include "qapi/qmp/qerror.h"
> > > > +#include "sysemu/cpus.h"
> > > >
> > > >  static bool vmstate_loading;
> > > >  static Notifier packets_compare_notifier;
> > > > @@ -414,23 +415,30 @@ static int colo_do_checkpoint_transaction
> (MigrationState
> > > *s,
> > > >
> > > >  /* Disable block migration */
> > > >  migrate_set_block_enabled(false, _err);
> > > > -qemu_savevm_state_header(fb);
> > > > -qemu_savevm_state_setup(fb);
> > > >  qemu_mutex_lock_iothread();
> > > >  replication_do_checkpoint_all(_err);
> > > >  if (local_err) {
> > > >  qemu_mutex_unlock_iothread();
> > > >  goto out;
> > > >  }
> > > > -qemu_savevm_state_complete_precopy(fb, false, false);
> > > > -qemu_mutex_unlock_iothread();
> > > > -
> > > > -qemu_fflush(fb);
> > > >
> > > >  colo_send_message(s->to_dst_file, COLO_MESSAGE_VMSTATE_SEND,
> > > _err);
> > > >  if (local_err) {
> > > >  goto out;
> > > >  }
> > > > +/*
> > > > + * Only save VM's live state, which not including device state.
> > > > + * TODO: We may need a timeout mechanism to prevent COLO process
> > > > + * to be blocked here.
> > > > + */
> > >
> > > I guess that's the downside to transmitting it directly than into the
> > > buffer;
> > > Peter Xu's OOB command system would let you kill the connection - and
> > > that's something I think COLO should use.
> > > Still the change saves you having that huge outgoing buffer on the
> > > source side and lets you start sending the checkpoint sooner, which
> > > means the pause time should be smaller.
> > >
> >
> > Yes, you are right.
> > But I think this is a performance optimization, this series focus on
> > enabling.
> > I will do this job in the future.
> >
> >
> > >
> > > > +qemu_savevm_live_state(s->to_dst_file);
> > >
> > > Does this actually need to be inside of the qemu_mutex_lock_iothread?
> > > I'm pretty sure the device_state needs to be, but I'm not sure the
> > > live_state needs to.
> > >
> >
> > I have checked the codes, qemu_savevm_live_state needn't inside of the
> > qemu_mutex_lock_iothread,
> > I will move the it out the lock area in next version.
> >
> >
> >
> > >
> > > > +/* Note: device state is saved into buffer */
> > > > +ret = qemu_save_device_state(fb);
> > > > +
> > > > +qemu_mutex_unlock_iothread();
> > > > +
> > > > +qemu_fflush(fb);
> > > > +
> > > >  /*
> > > >   * We need the size of the VMstate data in Secondary side,
> > > >   * With which we can decide how much data should be read.
> > > > @@ -643,6 +651,7 @@ void *colo_process_incoming_thread(void *opaque)
> > > >  uint64_t total_size;
> > > >  uint64_t value;
> > > >  Error *local_err = NULL;
> > > > +int ret;
> > > >
> > > >  qemu_sem_init(>colo_incoming_sem, 0);
> > > >
> > > > @@ -715,6 +724,16 @@ void *colo_process_incoming_thread(void
> *opaque)
> > > >  goto out;
> > > >  }
> > > >
> > > > +qemu_mutex_lock_iothread();
> > > > +cpu_synchronize_all_pre_loadvm();
> > > > +ret = qemu_loadvm_state_main(mis->from_src_file, mis);
> > > > +

Re: [Qemu-devel] [PATCH V7 RESEND 12/17] savevm: split the process of different stages for loadvm/savevm

2018-06-19 Thread Dr. David Alan Gilbert 
* Zhang Chen (zhangc...@gmail.com) wrote:
> On Wed, May 16, 2018 at 2:56 AM, Dr. David Alan Gilbert  > wrote:
> 
> > * Zhang Chen (zhangc...@gmail.com) wrote:
> > > From: zhanghailiang 
> > >
> > > There are several stages during loadvm/savevm process. In different
> > stage,
> > > migration incoming processes different types of sections.
> > > We want to control these stages more accuracy, it will benefit COLO
> > > performance, we don't have to save type of QEMU_VM_SECTION_START
> > > sections everytime while do checkpoint, besides, we want to separate
> > > the process of saving/loading memory and devices state.
> > >
> > > So we add three new helper functions: qemu_load_device_state() and
> > > qemu_savevm_live_state() to achieve different process during migration.
> > >
> > > Besides, we make qemu_loadvm_state_main() and qemu_save_device_state()
> > > public, and simplify the codes of qemu_save_device_state() by calling the
> > > wrapper qemu_savevm_state_header().
> > >
> > > Signed-off-by: zhanghailiang 
> > > Signed-off-by: Li Zhijian 
> > > Signed-off-by: Zhang Chen 
> > > Reviewed-by: Dr. David Alan Gilbert 
> > > ---
> > >  migration/colo.c   | 36 
> > >  migration/savevm.c | 35 ---
> > >  migration/savevm.h |  4 
> > >  3 files changed, 60 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/migration/colo.c b/migration/colo.c
> > > index cdff0a2490..5b055f79f1 100644
> > > --- a/migration/colo.c
> > > +++ b/migration/colo.c
> > > @@ -30,6 +30,7 @@
> > >  #include "block/block.h"
> > >  #include "qapi/qapi-events-migration.h"
> > >  #include "qapi/qmp/qerror.h"
> > > +#include "sysemu/cpus.h"
> > >
> > >  static bool vmstate_loading;
> > >  static Notifier packets_compare_notifier;
> > > @@ -414,23 +415,30 @@ static int 
> > > colo_do_checkpoint_transaction(MigrationState
> > *s,
> > >
> > >  /* Disable block migration */
> > >  migrate_set_block_enabled(false, _err);
> > > -qemu_savevm_state_header(fb);
> > > -qemu_savevm_state_setup(fb);
> > >  qemu_mutex_lock_iothread();
> > >  replication_do_checkpoint_all(_err);
> > >  if (local_err) {
> > >  qemu_mutex_unlock_iothread();
> > >  goto out;
> > >  }
> > > -qemu_savevm_state_complete_precopy(fb, false, false);
> > > -qemu_mutex_unlock_iothread();
> > > -
> > > -qemu_fflush(fb);
> > >
> > >  colo_send_message(s->to_dst_file, COLO_MESSAGE_VMSTATE_SEND,
> > _err);
> > >  if (local_err) {
> > >  goto out;
> > >  }
> > > +/*
> > > + * Only save VM's live state, which not including device state.
> > > + * TODO: We may need a timeout mechanism to prevent COLO process
> > > + * to be blocked here.
> > > + */
> >
> > I guess that's the downside to transmitting it directly than into the
> > buffer;
> > Peter Xu's OOB command system would let you kill the connection - and
> > that's something I think COLO should use.
> > Still the change saves you having that huge outgoing buffer on the
> > source side and lets you start sending the checkpoint sooner, which
> > means the pause time should be smaller.
> >
> 
> Yes, you are right.
> But I think this is a performance optimization, this series focus on
> enabling.
> I will do this job in the future.
> 
> 
> >
> > > +qemu_savevm_live_state(s->to_dst_file);
> >
> > Does this actually need to be inside of the qemu_mutex_lock_iothread?
> > I'm pretty sure the device_state needs to be, but I'm not sure the
> > live_state needs to.
> >
> 
> I have checked the codes, qemu_savevm_live_state needn't inside of the
> qemu_mutex_lock_iothread,
> I will move the it out the lock area in next version.
> 
> 
> 
> >
> > > +/* Note: device state is saved into buffer */
> > > +ret = qemu_save_device_state(fb);
> > > +
> > > +qemu_mutex_unlock_iothread();
> > > +
> > > +qemu_fflush(fb);
> > > +
> > >  /*
> > >   * We need the size of the VMstate data in Secondary side,
> > >   * With which we can decide how much data should be read.
> > > @@ -643,6 +651,7 @@ void *colo_process_incoming_thread(void *opaque)
> > >  uint64_t total_size;
> > >  uint64_t value;
> > >  Error *local_err = NULL;
> > > +int ret;
> > >
> > >  qemu_sem_init(>colo_incoming_sem, 0);
> > >
> > > @@ -715,6 +724,16 @@ void *colo_process_incoming_thread(void *opaque)
> > >  goto out;
> > >  }
> > >
> > > +qemu_mutex_lock_iothread();
> > > +cpu_synchronize_all_pre_loadvm();
> > > +ret = qemu_loadvm_state_main(mis->from_src_file, mis);
> > > +qemu_mutex_unlock_iothread();
> > > +
> > > +if (ret < 0) {
> > > +error_report("Load VM's live state (ram) error");
> > > +goto out;
> > > +}
> > > +
> > >  value = colo_receive_message_value(mis->from_src_file,
> > >   COLO_MESSAGE_VMSTATE_SIZE, _err);
> > >

Re: [Qemu-devel] [PATCH V7 RESEND 12/17] savevm: split the process of different stages for loadvm/savevm

2018-06-02 Thread Zhang Chen 
On Wed, May 16, 2018 at 2:56 AM, Dr. David Alan Gilbert  wrote:

> * Zhang Chen (zhangc...@gmail.com) wrote:
> > From: zhanghailiang 
> >
> > There are several stages during loadvm/savevm process. In different
> stage,
> > migration incoming processes different types of sections.
> > We want to control these stages more accuracy, it will benefit COLO
> > performance, we don't have to save type of QEMU_VM_SECTION_START
> > sections everytime while do checkpoint, besides, we want to separate
> > the process of saving/loading memory and devices state.
> >
> > So we add three new helper functions: qemu_load_device_state() and
> > qemu_savevm_live_state() to achieve different process during migration.
> >
> > Besides, we make qemu_loadvm_state_main() and qemu_save_device_state()
> > public, and simplify the codes of qemu_save_device_state() by calling the
> > wrapper qemu_savevm_state_header().
> >
> > Signed-off-by: zhanghailiang 
> > Signed-off-by: Li Zhijian 
> > Signed-off-by: Zhang Chen 
> > Reviewed-by: Dr. David Alan Gilbert 
> > ---
> >  migration/colo.c   | 36 
> >  migration/savevm.c | 35 ---
> >  migration/savevm.h |  4 
> >  3 files changed, 60 insertions(+), 15 deletions(-)
> >
> > diff --git a/migration/colo.c b/migration/colo.c
> > index cdff0a2490..5b055f79f1 100644
> > --- a/migration/colo.c
> > +++ b/migration/colo.c
> > @@ -30,6 +30,7 @@
> >  #include "block/block.h"
> >  #include "qapi/qapi-events-migration.h"
> >  #include "qapi/qmp/qerror.h"
> > +#include "sysemu/cpus.h"
> >
> >  static bool vmstate_loading;
> >  static Notifier packets_compare_notifier;
> > @@ -414,23 +415,30 @@ static int 
> > colo_do_checkpoint_transaction(MigrationState
> *s,
> >
> >  /* Disable block migration */
> >  migrate_set_block_enabled(false, _err);
> > -qemu_savevm_state_header(fb);
> > -qemu_savevm_state_setup(fb);
> >  qemu_mutex_lock_iothread();
> >  replication_do_checkpoint_all(_err);
> >  if (local_err) {
> >  qemu_mutex_unlock_iothread();
> >  goto out;
> >  }
> > -qemu_savevm_state_complete_precopy(fb, false, false);
> > -qemu_mutex_unlock_iothread();
> > -
> > -qemu_fflush(fb);
> >
> >  colo_send_message(s->to_dst_file, COLO_MESSAGE_VMSTATE_SEND,
> _err);
> >  if (local_err) {
> >  goto out;
> >  }
> > +/*
> > + * Only save VM's live state, which not including device state.
> > + * TODO: We may need a timeout mechanism to prevent COLO process
> > + * to be blocked here.
> > + */
>
> I guess that's the downside to transmitting it directly than into the
> buffer;
> Peter Xu's OOB command system would let you kill the connection - and
> that's something I think COLO should use.
> Still the change saves you having that huge outgoing buffer on the
> source side and lets you start sending the checkpoint sooner, which
> means the pause time should be smaller.
>

Yes, you are right.
But I think this is a performance optimization, this series focus on
enabling.
I will do this job in the future.


>
> > +qemu_savevm_live_state(s->to_dst_file);
>
> Does this actually need to be inside of the qemu_mutex_lock_iothread?
> I'm pretty sure the device_state needs to be, but I'm not sure the
> live_state needs to.
>

I have checked the codes, qemu_savevm_live_state needn't inside of the
qemu_mutex_lock_iothread,
I will move the it out the lock area in next version.



>
> > +/* Note: device state is saved into buffer */
> > +ret = qemu_save_device_state(fb);
> > +
> > +qemu_mutex_unlock_iothread();
> > +
> > +qemu_fflush(fb);
> > +
> >  /*
> >   * We need the size of the VMstate data in Secondary side,
> >   * With which we can decide how much data should be read.
> > @@ -643,6 +651,7 @@ void *colo_process_incoming_thread(void *opaque)
> >  uint64_t total_size;
> >  uint64_t value;
> >  Error *local_err = NULL;
> > +int ret;
> >
> >  qemu_sem_init(>colo_incoming_sem, 0);
> >
> > @@ -715,6 +724,16 @@ void *colo_process_incoming_thread(void *opaque)
> >  goto out;
> >  }
> >
> > +qemu_mutex_lock_iothread();
> > +cpu_synchronize_all_pre_loadvm();
> > +ret = qemu_loadvm_state_main(mis->from_src_file, mis);
> > +qemu_mutex_unlock_iothread();
> > +
> > +if (ret < 0) {
> > +error_report("Load VM's live state (ram) error");
> > +goto out;
> > +}
> > +
> >  value = colo_receive_message_value(mis->from_src_file,
> >   COLO_MESSAGE_VMSTATE_SIZE, _err);
> >  if (local_err) {
> > @@ -748,8 +767,9 @@ void *colo_process_incoming_thread(void *opaque)
> >  qemu_mutex_lock_iothread();
> >  qemu_system_reset(SHUTDOWN_CAUSE_NONE);
>
> Is the reset safe? Are you sure it doesn't change the ram you've just
> loaded?
>

Yes, It is safe. In my test the secondary node

Re: [Qemu-devel] [PATCH V7 RESEND 12/17] savevm: split the process of different stages for loadvm/savevm

2018-05-15 Thread Dr. David Alan Gilbert 
* Zhang Chen (zhangc...@gmail.com) wrote:
> From: zhanghailiang 
> 
> There are several stages during loadvm/savevm process. In different stage,
> migration incoming processes different types of sections.
> We want to control these stages more accuracy, it will benefit COLO
> performance, we don't have to save type of QEMU_VM_SECTION_START
> sections everytime while do checkpoint, besides, we want to separate
> the process of saving/loading memory and devices state.
> 
> So we add three new helper functions: qemu_load_device_state() and
> qemu_savevm_live_state() to achieve different process during migration.
> 
> Besides, we make qemu_loadvm_state_main() and qemu_save_device_state()
> public, and simplify the codes of qemu_save_device_state() by calling the
> wrapper qemu_savevm_state_header().
> 
> Signed-off-by: zhanghailiang 
> Signed-off-by: Li Zhijian 
> Signed-off-by: Zhang Chen 
> Reviewed-by: Dr. David Alan Gilbert 
> ---
>  migration/colo.c   | 36 
>  migration/savevm.c | 35 ---
>  migration/savevm.h |  4 
>  3 files changed, 60 insertions(+), 15 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index cdff0a2490..5b055f79f1 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -30,6 +30,7 @@
>  #include "block/block.h"
>  #include "qapi/qapi-events-migration.h"
>  #include "qapi/qmp/qerror.h"
> +#include "sysemu/cpus.h"
>  
>  static bool vmstate_loading;
>  static Notifier packets_compare_notifier;
> @@ -414,23 +415,30 @@ static int 
> colo_do_checkpoint_transaction(MigrationState *s,
>  
>  /* Disable block migration */
>  migrate_set_block_enabled(false, _err);
> -qemu_savevm_state_header(fb);
> -qemu_savevm_state_setup(fb);
>  qemu_mutex_lock_iothread();
>  replication_do_checkpoint_all(_err);
>  if (local_err) {
>  qemu_mutex_unlock_iothread();
>  goto out;
>  }
> -qemu_savevm_state_complete_precopy(fb, false, false);
> -qemu_mutex_unlock_iothread();
> -
> -qemu_fflush(fb);
>  
>  colo_send_message(s->to_dst_file, COLO_MESSAGE_VMSTATE_SEND, _err);
>  if (local_err) {
>  goto out;
>  }
> +/*
> + * Only save VM's live state, which not including device state.
> + * TODO: We may need a timeout mechanism to prevent COLO process
> + * to be blocked here.
> + */

I guess that's the downside to transmitting it directly than into the buffer;
Peter Xu's OOB command system would let you kill the connection - and
that's something I think COLO should use.
Still the change saves you having that huge outgoing buffer on the
source side and lets you start sending the checkpoint sooner, which
means the pause time should be smaller.

> +qemu_savevm_live_state(s->to_dst_file);

Does this actually need to be inside of the qemu_mutex_lock_iothread?
I'm pretty sure the device_state needs to be, but I'm not sure the
live_state needs to.

> +/* Note: device state is saved into buffer */
> +ret = qemu_save_device_state(fb);
> +
> +qemu_mutex_unlock_iothread();
> +
> +qemu_fflush(fb);
> +
>  /*
>   * We need the size of the VMstate data in Secondary side,
>   * With which we can decide how much data should be read.
> @@ -643,6 +651,7 @@ void *colo_process_incoming_thread(void *opaque)
>  uint64_t total_size;
>  uint64_t value;
>  Error *local_err = NULL;
> +int ret;
>  
>  qemu_sem_init(>colo_incoming_sem, 0);
>  
> @@ -715,6 +724,16 @@ void *colo_process_incoming_thread(void *opaque)
>  goto out;
>  }
>  
> +qemu_mutex_lock_iothread();
> +cpu_synchronize_all_pre_loadvm();
> +ret = qemu_loadvm_state_main(mis->from_src_file, mis);
> +qemu_mutex_unlock_iothread();
> +
> +if (ret < 0) {
> +error_report("Load VM's live state (ram) error");
> +goto out;
> +}
> +
>  value = colo_receive_message_value(mis->from_src_file,
>   COLO_MESSAGE_VMSTATE_SIZE, _err);
>  if (local_err) {
> @@ -748,8 +767,9 @@ void *colo_process_incoming_thread(void *opaque)
>  qemu_mutex_lock_iothread();
>  qemu_system_reset(SHUTDOWN_CAUSE_NONE);

Is the reset safe? Are you sure it doesn't change the ram you've just
loaded?

>  vmstate_loading = true;
> -if (qemu_loadvm_state(fb) < 0) {
> -error_report("COLO: loadvm failed");
> +ret = qemu_load_device_state(fb);
> +if (ret < 0) {
> +error_report("COLO: load device state failed");
>  qemu_mutex_unlock_iothread();
>  goto out;
>  }
> diff --git a/migration/savevm.c b/migration/savevm.c
> index ec0bff09ce..0f61239429 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@