Re: [Qemu-devel] [RFC 12/29] migration: allow dst vm pause on postcopy

2017-08-04 Thread Peter Xu 
On Fri, Aug 04, 2017 at 10:33:19AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > On Thu, Aug 03, 2017 at 03:03:57PM +0100, Dr. David Alan Gilbert wrote:
> > > * Peter Xu (pet...@redhat.com) wrote:
> > > > On Tue, Aug 01, 2017 at 10:47:16AM +0100, Dr. David Alan Gilbert wrote:
> > > > > * Peter Xu (pet...@redhat.com) wrote:
> > > > 
> > > > [...]
> > > > 
> > > > > > +/* Return true if we should continue the migration, or false. */
> > > > > > +static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> > > > > > +{
> > > > > > +trace_postcopy_pause_incoming();
> > > > > > +
> > > > > > +migrate_set_state(>state, 
> > > > > > MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > > > > > +  MIGRATION_STATUS_POSTCOPY_PAUSED);
> > > > > > +
> > > > > > +assert(mis->from_src_file);
> > > > > > +qemu_file_shutdown(mis->from_src_file);
> > > > > > +qemu_fclose(mis->from_src_file);
> > > > > > +mis->from_src_file = NULL;
> > > > > > +
> > > > > > +assert(mis->to_src_file);
> > > > > > +qemu_mutex_lock(>rp_mutex);
> > > > > > +qemu_file_shutdown(mis->to_src_file);
> > > > > > +qemu_fclose(mis->to_src_file);
> > > > > > +mis->to_src_file = NULL;
> > > > > > +qemu_mutex_unlock(>rp_mutex);
> > > > > 
> > > > > Hmm is that safe?  If we look at migrate_send_rp_message we have:
> > > > > 
> > > > > static void migrate_send_rp_message(MigrationIncomingState *mis,
> > > > > enum mig_rp_message_type 
> > > > > message_type,
> > > > > uint16_t len, void *data)
> > > > > {
> > > > > trace_migrate_send_rp_message((int)message_type, len);
> > > > > qemu_mutex_lock(>rp_mutex);
> > > > > qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
> > > > > qemu_put_be16(mis->to_src_file, len);
> > > > > qemu_put_buffer(mis->to_src_file, data, len);
> > > > > qemu_fflush(mis->to_src_file);
> > > > > qemu_mutex_unlock(>rp_mutex);
> > > > > }
> > > > > 
> > > > > If we came into postcopy_pause_incoming at about the same time
> > > > > migrate_send_rp_message was being called and pause_incoming took the
> > > > > lock first, then once it release the lock, send_rp_message carries on
> > > > > and uses mis->to_src_file that's now NULL.
> > > > > 
> > > > > One solution here is to just call qemu_file_shutdown() but leave the
> > > > > files open at this point, but clean the files up sometime later.
> > > > 
> > > > I see the commnent on patch 14 as well - yeah, we need patch 14 to
> > > > co-op here, and as long as we are with patch 14, we should be ok.
> > > > 
> > > > > 
> > > > > > +
> > > > > > +while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > > > > > +qemu_sem_wait(>postcopy_pause_sem_dst);
> > > > > > +}
> > > > > > +
> > > > > > +trace_postcopy_pause_incoming_continued();
> > > > > > +
> > > > > > +return true;
> > > > > > +}
> > > > > > +
> > > > > >  static int qemu_loadvm_state_main(QEMUFile *f, 
> > > > > > MigrationIncomingState *mis)
> > > > > >  {
> > > > > >  uint8_t section_type;
> > > > > >  int ret = 0;
> > > > > >  
> > > > > > +retry:
> > > > > >  while (true) {
> > > > > >  section_type = qemu_get_byte(f);
> > > > > >  
> > > > > > @@ -2004,6 +2034,21 @@ static int qemu_loadvm_state_main(QEMUFile 
> > > > > > *f, MigrationIncomingState *mis)
> > > > > >  out:
> > > > > >  if (ret < 0) {
> > > > > >  qemu_file_set_error(f, ret);
> > > > > > +
> > > > > > +/*
> > > > > > + * Detect whether it is:
> > > > > > + *
> > > > > > + * 1. postcopy running
> > > > > > + * 2. network failure (-EIO)
> > > > > > + *
> > > > > > + * If so, we try to wait for a recovery.
> > > > > > + */
> > > > > > +if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > > > > > +ret == -EIO && postcopy_pause_incoming(mis)) {
> > > > > > +/* Reset f to point to the newly created channel */
> > > > > > +f = mis->from_src_file;
> > > > > > +goto retry;
> > > > > > +}
> > > > > 
> > > > > I wonder if:
> > > > > 
> > > > >if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > > > >ret == -EIO && postcopy_pause_incoming(mis)) {
> > > > >/* Try again after postcopy recovery */
> > > > >return qemu_loadvm_state_main(mis->from_src_file, mis);
> > > > >}
> > > > > would be nicer; it avoids the goto loop.
> > > > 
> > > > I agree we should avoid using goto loops. However I do see vast usages
> > > > of goto like this one when we want to redo part of the procedures of a
> > > > function (or, of course, when handling errors in "C-style").
> > > 
> > > We mostly use them to jump forward to an error exit; only rarely do
> > > we do loops with them;  so

Re: [Qemu-devel] [RFC 12/29] migration: allow dst vm pause on postcopy

2017-08-04 Thread Dr. David Alan Gilbert 
* Peter Xu (pet...@redhat.com) wrote:
> On Thu, Aug 03, 2017 at 03:03:57PM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (pet...@redhat.com) wrote:
> > > On Tue, Aug 01, 2017 at 10:47:16AM +0100, Dr. David Alan Gilbert wrote:
> > > > * Peter Xu (pet...@redhat.com) wrote:
> > > 
> > > [...]
> > > 
> > > > > +/* Return true if we should continue the migration, or false. */
> > > > > +static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> > > > > +{
> > > > > +trace_postcopy_pause_incoming();
> > > > > +
> > > > > +migrate_set_state(>state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > > > > +  MIGRATION_STATUS_POSTCOPY_PAUSED);
> > > > > +
> > > > > +assert(mis->from_src_file);
> > > > > +qemu_file_shutdown(mis->from_src_file);
> > > > > +qemu_fclose(mis->from_src_file);
> > > > > +mis->from_src_file = NULL;
> > > > > +
> > > > > +assert(mis->to_src_file);
> > > > > +qemu_mutex_lock(>rp_mutex);
> > > > > +qemu_file_shutdown(mis->to_src_file);
> > > > > +qemu_fclose(mis->to_src_file);
> > > > > +mis->to_src_file = NULL;
> > > > > +qemu_mutex_unlock(>rp_mutex);
> > > > 
> > > > Hmm is that safe?  If we look at migrate_send_rp_message we have:
> > > > 
> > > > static void migrate_send_rp_message(MigrationIncomingState *mis,
> > > > enum mig_rp_message_type 
> > > > message_type,
> > > > uint16_t len, void *data)
> > > > {
> > > > trace_migrate_send_rp_message((int)message_type, len);
> > > > qemu_mutex_lock(>rp_mutex);
> > > > qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
> > > > qemu_put_be16(mis->to_src_file, len);
> > > > qemu_put_buffer(mis->to_src_file, data, len);
> > > > qemu_fflush(mis->to_src_file);
> > > > qemu_mutex_unlock(>rp_mutex);
> > > > }
> > > > 
> > > > If we came into postcopy_pause_incoming at about the same time
> > > > migrate_send_rp_message was being called and pause_incoming took the
> > > > lock first, then once it release the lock, send_rp_message carries on
> > > > and uses mis->to_src_file that's now NULL.
> > > > 
> > > > One solution here is to just call qemu_file_shutdown() but leave the
> > > > files open at this point, but clean the files up sometime later.
> > > 
> > > I see the commnent on patch 14 as well - yeah, we need patch 14 to
> > > co-op here, and as long as we are with patch 14, we should be ok.
> > > 
> > > > 
> > > > > +
> > > > > +while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > > > > +qemu_sem_wait(>postcopy_pause_sem_dst);
> > > > > +}
> > > > > +
> > > > > +trace_postcopy_pause_incoming_continued();
> > > > > +
> > > > > +return true;
> > > > > +}
> > > > > +
> > > > >  static int qemu_loadvm_state_main(QEMUFile *f, 
> > > > > MigrationIncomingState *mis)
> > > > >  {
> > > > >  uint8_t section_type;
> > > > >  int ret = 0;
> > > > >  
> > > > > +retry:
> > > > >  while (true) {
> > > > >  section_type = qemu_get_byte(f);
> > > > >  
> > > > > @@ -2004,6 +2034,21 @@ static int qemu_loadvm_state_main(QEMUFile *f, 
> > > > > MigrationIncomingState *mis)
> > > > >  out:
> > > > >  if (ret < 0) {
> > > > >  qemu_file_set_error(f, ret);
> > > > > +
> > > > > +/*
> > > > > + * Detect whether it is:
> > > > > + *
> > > > > + * 1. postcopy running
> > > > > + * 2. network failure (-EIO)
> > > > > + *
> > > > > + * If so, we try to wait for a recovery.
> > > > > + */
> > > > > +if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > > > > +ret == -EIO && postcopy_pause_incoming(mis)) {
> > > > > +/* Reset f to point to the newly created channel */
> > > > > +f = mis->from_src_file;
> > > > > +goto retry;
> > > > > +}
> > > > 
> > > > I wonder if:
> > > > 
> > > >if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > > >ret == -EIO && postcopy_pause_incoming(mis)) {
> > > >/* Try again after postcopy recovery */
> > > >return qemu_loadvm_state_main(mis->from_src_file, mis);
> > > >}
> > > > would be nicer; it avoids the goto loop.
> > > 
> > > I agree we should avoid using goto loops. However I do see vast usages
> > > of goto like this one when we want to redo part of the procedures of a
> > > function (or, of course, when handling errors in "C-style").
> > 
> > We mostly use them to jump forward to an error exit; only rarely do
> > we do loops with them;  so if we can sensibly avoid them it's best.
> > 
> > > Calling qemu_loadvm_state_main() inside itself is ok as well, but it
> > > also has defect: stack usage would be out of control, or even, it can
> > > be controled by malicious users. E.g., if someone used program to
> > > periodically stop/start any network

Re: [Qemu-devel] [RFC 12/29] migration: allow dst vm pause on postcopy

2017-08-03 Thread Peter Xu 
On Thu, Aug 03, 2017 at 03:03:57PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > On Tue, Aug 01, 2017 at 10:47:16AM +0100, Dr. David Alan Gilbert wrote:
> > > * Peter Xu (pet...@redhat.com) wrote:
> > 
> > [...]
> > 
> > > > +/* Return true if we should continue the migration, or false. */
> > > > +static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> > > > +{
> > > > +trace_postcopy_pause_incoming();
> > > > +
> > > > +migrate_set_state(>state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > > > +  MIGRATION_STATUS_POSTCOPY_PAUSED);
> > > > +
> > > > +assert(mis->from_src_file);
> > > > +qemu_file_shutdown(mis->from_src_file);
> > > > +qemu_fclose(mis->from_src_file);
> > > > +mis->from_src_file = NULL;
> > > > +
> > > > +assert(mis->to_src_file);
> > > > +qemu_mutex_lock(>rp_mutex);
> > > > +qemu_file_shutdown(mis->to_src_file);
> > > > +qemu_fclose(mis->to_src_file);
> > > > +mis->to_src_file = NULL;
> > > > +qemu_mutex_unlock(>rp_mutex);
> > > 
> > > Hmm is that safe?  If we look at migrate_send_rp_message we have:
> > > 
> > > static void migrate_send_rp_message(MigrationIncomingState *mis,
> > > enum mig_rp_message_type 
> > > message_type,
> > > uint16_t len, void *data)
> > > {
> > > trace_migrate_send_rp_message((int)message_type, len);
> > > qemu_mutex_lock(>rp_mutex);
> > > qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
> > > qemu_put_be16(mis->to_src_file, len);
> > > qemu_put_buffer(mis->to_src_file, data, len);
> > > qemu_fflush(mis->to_src_file);
> > > qemu_mutex_unlock(>rp_mutex);
> > > }
> > > 
> > > If we came into postcopy_pause_incoming at about the same time
> > > migrate_send_rp_message was being called and pause_incoming took the
> > > lock first, then once it release the lock, send_rp_message carries on
> > > and uses mis->to_src_file that's now NULL.
> > > 
> > > One solution here is to just call qemu_file_shutdown() but leave the
> > > files open at this point, but clean the files up sometime later.
> > 
> > I see the commnent on patch 14 as well - yeah, we need patch 14 to
> > co-op here, and as long as we are with patch 14, we should be ok.
> > 
> > > 
> > > > +
> > > > +while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > > > +qemu_sem_wait(>postcopy_pause_sem_dst);
> > > > +}
> > > > +
> > > > +trace_postcopy_pause_incoming_continued();
> > > > +
> > > > +return true;
> > > > +}
> > > > +
> > > >  static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState 
> > > > *mis)
> > > >  {
> > > >  uint8_t section_type;
> > > >  int ret = 0;
> > > >  
> > > > +retry:
> > > >  while (true) {
> > > >  section_type = qemu_get_byte(f);
> > > >  
> > > > @@ -2004,6 +2034,21 @@ static int qemu_loadvm_state_main(QEMUFile *f, 
> > > > MigrationIncomingState *mis)
> > > >  out:
> > > >  if (ret < 0) {
> > > >  qemu_file_set_error(f, ret);
> > > > +
> > > > +/*
> > > > + * Detect whether it is:
> > > > + *
> > > > + * 1. postcopy running
> > > > + * 2. network failure (-EIO)
> > > > + *
> > > > + * If so, we try to wait for a recovery.
> > > > + */
> > > > +if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > > > +ret == -EIO && postcopy_pause_incoming(mis)) {
> > > > +/* Reset f to point to the newly created channel */
> > > > +f = mis->from_src_file;
> > > > +goto retry;
> > > > +}
> > > 
> > > I wonder if:
> > > 
> > >if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > >ret == -EIO && postcopy_pause_incoming(mis)) {
> > >/* Try again after postcopy recovery */
> > >return qemu_loadvm_state_main(mis->from_src_file, mis);
> > >}
> > > would be nicer; it avoids the goto loop.
> > 
> > I agree we should avoid using goto loops. However I do see vast usages
> > of goto like this one when we want to redo part of the procedures of a
> > function (or, of course, when handling errors in "C-style").
> 
> We mostly use them to jump forward to an error exit; only rarely do
> we do loops with them;  so if we can sensibly avoid them it's best.
> 
> > Calling qemu_loadvm_state_main() inside itself is ok as well, but it
> > also has defect: stack usage would be out of control, or even, it can
> > be controled by malicious users. E.g., if someone used program to
> > periodically stop/start any network endpoint along the migration
> > network, QEMU may go into a paused -> recovery -> active -> paused ...
> > loop, and stack usage will just grow with time. I'd say it's an
> > extreme example though...
> 
> I think it's safe because it's a tail-call so a new stack

Re: [Qemu-devel] [RFC 12/29] migration: allow dst vm pause on postcopy

2017-08-03 Thread Dr. David Alan Gilbert 
* Peter Xu (pet...@redhat.com) wrote:
> On Tue, Aug 01, 2017 at 10:47:16AM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (pet...@redhat.com) wrote:
> 
> [...]
> 
> > > +/* Return true if we should continue the migration, or false. */
> > > +static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> > > +{
> > > +trace_postcopy_pause_incoming();
> > > +
> > > +migrate_set_state(>state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > > +  MIGRATION_STATUS_POSTCOPY_PAUSED);
> > > +
> > > +assert(mis->from_src_file);
> > > +qemu_file_shutdown(mis->from_src_file);
> > > +qemu_fclose(mis->from_src_file);
> > > +mis->from_src_file = NULL;
> > > +
> > > +assert(mis->to_src_file);
> > > +qemu_mutex_lock(>rp_mutex);
> > > +qemu_file_shutdown(mis->to_src_file);
> > > +qemu_fclose(mis->to_src_file);
> > > +mis->to_src_file = NULL;
> > > +qemu_mutex_unlock(>rp_mutex);
> > 
> > Hmm is that safe?  If we look at migrate_send_rp_message we have:
> > 
> > static void migrate_send_rp_message(MigrationIncomingState *mis,
> > enum mig_rp_message_type 
> > message_type,
> > uint16_t len, void *data)
> > {
> > trace_migrate_send_rp_message((int)message_type, len);
> > qemu_mutex_lock(>rp_mutex);
> > qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
> > qemu_put_be16(mis->to_src_file, len);
> > qemu_put_buffer(mis->to_src_file, data, len);
> > qemu_fflush(mis->to_src_file);
> > qemu_mutex_unlock(>rp_mutex);
> > }
> > 
> > If we came into postcopy_pause_incoming at about the same time
> > migrate_send_rp_message was being called and pause_incoming took the
> > lock first, then once it release the lock, send_rp_message carries on
> > and uses mis->to_src_file that's now NULL.
> > 
> > One solution here is to just call qemu_file_shutdown() but leave the
> > files open at this point, but clean the files up sometime later.
> 
> I see the commnent on patch 14 as well - yeah, we need patch 14 to
> co-op here, and as long as we are with patch 14, we should be ok.
> 
> > 
> > > +
> > > +while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > > +qemu_sem_wait(>postcopy_pause_sem_dst);
> > > +}
> > > +
> > > +trace_postcopy_pause_incoming_continued();
> > > +
> > > +return true;
> > > +}
> > > +
> > >  static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState 
> > > *mis)
> > >  {
> > >  uint8_t section_type;
> > >  int ret = 0;
> > >  
> > > +retry:
> > >  while (true) {
> > >  section_type = qemu_get_byte(f);
> > >  
> > > @@ -2004,6 +2034,21 @@ static int qemu_loadvm_state_main(QEMUFile *f, 
> > > MigrationIncomingState *mis)
> > >  out:
> > >  if (ret < 0) {
> > >  qemu_file_set_error(f, ret);
> > > +
> > > +/*
> > > + * Detect whether it is:
> > > + *
> > > + * 1. postcopy running
> > > + * 2. network failure (-EIO)
> > > + *
> > > + * If so, we try to wait for a recovery.
> > > + */
> > > +if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > > +ret == -EIO && postcopy_pause_incoming(mis)) {
> > > +/* Reset f to point to the newly created channel */
> > > +f = mis->from_src_file;
> > > +goto retry;
> > > +}
> > 
> > I wonder if:
> > 
> >if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> >ret == -EIO && postcopy_pause_incoming(mis)) {
> >/* Try again after postcopy recovery */
> >return qemu_loadvm_state_main(mis->from_src_file, mis);
> >}
> > would be nicer; it avoids the goto loop.
> 
> I agree we should avoid using goto loops. However I do see vast usages
> of goto like this one when we want to redo part of the procedures of a
> function (or, of course, when handling errors in "C-style").

We mostly use them to jump forward to an error exit; only rarely do
we do loops with them;  so if we can sensibly avoid them it's best.

> Calling qemu_loadvm_state_main() inside itself is ok as well, but it
> also has defect: stack usage would be out of control, or even, it can
> be controled by malicious users. E.g., if someone used program to
> periodically stop/start any network endpoint along the migration
> network, QEMU may go into a paused -> recovery -> active -> paused ...
> loop, and stack usage will just grow with time. I'd say it's an
> extreme example though...

I think it's safe because it's a tail-call so a new stack frame isn't
needed.

> (Another way besides above two: maybe we can just return in
>  qemu_loadvm_state_main with something like -EAGAIN, then the caller
>  of qemu_loadvm_state_main can re-call it when necessary, though I
>  would prefer "goto is okay here"... :)

Dave

> -- 
> Peter Xu
--
Dr. David Alan Gilbert /

Re: [Qemu-devel] [RFC 12/29] migration: allow dst vm pause on postcopy

2017-08-01 Thread Peter Xu 
On Tue, Aug 01, 2017 at 10:47:16AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:

[...]

> > +/* Return true if we should continue the migration, or false. */
> > +static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> > +{
> > +trace_postcopy_pause_incoming();
> > +
> > +migrate_set_state(>state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > +  MIGRATION_STATUS_POSTCOPY_PAUSED);
> > +
> > +assert(mis->from_src_file);
> > +qemu_file_shutdown(mis->from_src_file);
> > +qemu_fclose(mis->from_src_file);
> > +mis->from_src_file = NULL;
> > +
> > +assert(mis->to_src_file);
> > +qemu_mutex_lock(>rp_mutex);
> > +qemu_file_shutdown(mis->to_src_file);
> > +qemu_fclose(mis->to_src_file);
> > +mis->to_src_file = NULL;
> > +qemu_mutex_unlock(>rp_mutex);
> 
> Hmm is that safe?  If we look at migrate_send_rp_message we have:
> 
> static void migrate_send_rp_message(MigrationIncomingState *mis,
> enum mig_rp_message_type message_type,
> uint16_t len, void *data)
> {
> trace_migrate_send_rp_message((int)message_type, len);
> qemu_mutex_lock(>rp_mutex);
> qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
> qemu_put_be16(mis->to_src_file, len);
> qemu_put_buffer(mis->to_src_file, data, len);
> qemu_fflush(mis->to_src_file);
> qemu_mutex_unlock(>rp_mutex);
> }
> 
> If we came into postcopy_pause_incoming at about the same time
> migrate_send_rp_message was being called and pause_incoming took the
> lock first, then once it release the lock, send_rp_message carries on
> and uses mis->to_src_file that's now NULL.
> 
> One solution here is to just call qemu_file_shutdown() but leave the
> files open at this point, but clean the files up sometime later.

I see the commnent on patch 14 as well - yeah, we need patch 14 to
co-op here, and as long as we are with patch 14, we should be ok.

> 
> > +
> > +while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > +qemu_sem_wait(>postcopy_pause_sem_dst);
> > +}
> > +
> > +trace_postcopy_pause_incoming_continued();
> > +
> > +return true;
> > +}
> > +
> >  static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> >  {
> >  uint8_t section_type;
> >  int ret = 0;
> >  
> > +retry:
> >  while (true) {
> >  section_type = qemu_get_byte(f);
> >  
> > @@ -2004,6 +2034,21 @@ static int qemu_loadvm_state_main(QEMUFile *f, 
> > MigrationIncomingState *mis)
> >  out:
> >  if (ret < 0) {
> >  qemu_file_set_error(f, ret);
> > +
> > +/*
> > + * Detect whether it is:
> > + *
> > + * 1. postcopy running
> > + * 2. network failure (-EIO)
> > + *
> > + * If so, we try to wait for a recovery.
> > + */
> > +if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > +ret == -EIO && postcopy_pause_incoming(mis)) {
> > +/* Reset f to point to the newly created channel */
> > +f = mis->from_src_file;
> > +goto retry;
> > +}
> 
> I wonder if:
> 
>if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
>ret == -EIO && postcopy_pause_incoming(mis)) {
>/* Try again after postcopy recovery */
>return qemu_loadvm_state_main(mis->from_src_file, mis);
>}
> would be nicer; it avoids the goto loop.

I agree we should avoid using goto loops. However I do see vast usages
of goto like this one when we want to redo part of the procedures of a
function (or, of course, when handling errors in "C-style").

Calling qemu_loadvm_state_main() inside itself is ok as well, but it
also has defect: stack usage would be out of control, or even, it can
be controled by malicious users. E.g., if someone used program to
periodically stop/start any network endpoint along the migration
network, QEMU may go into a paused -> recovery -> active -> paused ...
loop, and stack usage will just grow with time. I'd say it's an
extreme example though...

(Another way besides above two: maybe we can just return in
 qemu_loadvm_state_main with something like -EAGAIN, then the caller
 of qemu_loadvm_state_main can re-call it when necessary, though I
 would prefer "goto is okay here"... :)

-- 
Peter Xu

Re: [Qemu-devel] [RFC 12/29] migration: allow dst vm pause on postcopy

2017-08-01 Thread Dr. David Alan Gilbert 
* Peter Xu (pet...@redhat.com) wrote:
> When there is IO error on the incoming channel (e.g., network down),
> instead of bailing out immediately, we allow the dst vm to switch to the
> new POSTCOPY_PAUSE state. Currently it is still simple - it waits the
> new semaphore, until someone poke it for another attempt.
> 
> Signed-off-by: Peter Xu 
> ---
>  migration/migration.c  |  1 +
>  migration/migration.h  |  3 +++
>  migration/savevm.c | 45 +
>  migration/trace-events |  2 ++
>  4 files changed, 51 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 0bc70c8..c729c5a 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -146,6 +146,7 @@ MigrationIncomingState 
> *migration_incoming_get_current(void)
>  memset(_current, 0, sizeof(MigrationIncomingState));
>  qemu_mutex_init(_current.rp_mutex);
>  qemu_event_init(_current.main_thread_load_event, false);
> +qemu_sem_init(_current.postcopy_pause_sem_dst, 0);
>  once = true;
>  }
>  return _current;
> diff --git a/migration/migration.h b/migration/migration.h
> index 24cdaf6..08b90e8 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -60,6 +60,9 @@ struct MigrationIncomingState {
>  /* The coroutine we should enter (back) after failover */
>  Coroutine *migration_incoming_co;
>  QemuSemaphore colo_incoming_sem;
> +
> +/* notify PAUSED postcopy incoming migrations to try to continue */
> +QemuSemaphore postcopy_pause_sem_dst;
>  };
>  
>  MigrationIncomingState *migration_incoming_get_current(void);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 13ae9d6..1f62268 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1954,11 +1954,41 @@ void qemu_loadvm_state_cleanup(void)
>  }
>  }
>  
> +/* Return true if we should continue the migration, or false. */
> +static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> +{
> +trace_postcopy_pause_incoming();
> +
> +migrate_set_state(>state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> +  MIGRATION_STATUS_POSTCOPY_PAUSED);
> +
> +assert(mis->from_src_file);
> +qemu_file_shutdown(mis->from_src_file);
> +qemu_fclose(mis->from_src_file);
> +mis->from_src_file = NULL;
> +
> +assert(mis->to_src_file);
> +qemu_mutex_lock(>rp_mutex);
> +qemu_file_shutdown(mis->to_src_file);
> +qemu_fclose(mis->to_src_file);
> +mis->to_src_file = NULL;
> +qemu_mutex_unlock(>rp_mutex);

Hmm is that safe?  If we look at migrate_send_rp_message we have:

static void migrate_send_rp_message(MigrationIncomingState *mis,
enum mig_rp_message_type message_type,
uint16_t len, void *data)
{
trace_migrate_send_rp_message((int)message_type, len);
qemu_mutex_lock(>rp_mutex);
qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
qemu_put_be16(mis->to_src_file, len);
qemu_put_buffer(mis->to_src_file, data, len);
qemu_fflush(mis->to_src_file);
qemu_mutex_unlock(>rp_mutex);
}

If we came into postcopy_pause_incoming at about the same time
migrate_send_rp_message was being called and pause_incoming took the
lock first, then once it release the lock, send_rp_message carries on
and uses mis->to_src_file that's now NULL.

One solution here is to just call qemu_file_shutdown() but leave the
files open at this point, but clean the files up sometime later.

> +
> +while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> +qemu_sem_wait(>postcopy_pause_sem_dst);
> +}
> +
> +trace_postcopy_pause_incoming_continued();
> +
> +return true;
> +}
> +
>  static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
>  {
>  uint8_t section_type;
>  int ret = 0;
>  
> +retry:
>  while (true) {
>  section_type = qemu_get_byte(f);
>  
> @@ -2004,6 +2034,21 @@ static int qemu_loadvm_state_main(QEMUFile *f, 
> MigrationIncomingState *mis)
>  out:
>  if (ret < 0) {
>  qemu_file_set_error(f, ret);
> +
> +/*
> + * Detect whether it is:
> + *
> + * 1. postcopy running
> + * 2. network failure (-EIO)
> + *
> + * If so, we try to wait for a recovery.
> + */
> +if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> +ret == -EIO && postcopy_pause_incoming(mis)) {
> +/* Reset f to point to the newly created channel */
> +f = mis->from_src_file;
> +goto retry;
> +}

I wonder if:

   if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
   ret == -EIO && postcopy_pause_incoming(mis)) {
   /* Try again after postcopy recovery */
   return qemu_loadvm_state_main(mis->from_src_file, mis);
   }