Re: [Qemu-devel] [PATCH v5 15/45] Rework loadvm path for subloops

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* David Gibson (david@gibson.dropbear.id.au) wrote:
> On Wed, Feb 25, 2015 at 04:51:38PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Postcopy needs to have two migration streams loading concurrently;
> > one from memory (with the device state) and the other from the fd
> > with the memory transactions.
> > 
> > Split the core of qemu_loadvm_state out so we can use it for both.
> > 
> > Allow the inner loadvm loop to quit and signal whether the parent
> > should.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  savevm.c     | 106 ++++++++++++++++++++++++++++++++++++-----------------------
> >  trace-events |   4 +++
> >  2 files changed, 69 insertions(+), 41 deletions(-)
> > 
> > diff --git a/savevm.c b/savevm.c
> > index f42713d..4b619da 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -951,6 +951,16 @@ static SaveStateEntry *find_se(const char *idstr, int instance_id)
> >      return NULL;
> >  }
> >  
> > +/* ORable flags that control the (potentially nested) loadvm_state loops */
> > +enum LoadVMExitCodes {
> > +    /* Quit the loop level that received this command */
> > +    LOADVM_QUIT_LOOP     =  1,
> > +    /* Quit this loop and our parent */
> > +    LOADVM_QUIT_PARENT   =  2,
> > +};
> 
> The semantics of all the exit code stuff is doing my head in; I'm not
> sure how to make it more comprehensible.

I've managed to kill one of the states off; it's now a single flag that quits
all levels; much easier to understand.

> > +static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis);
> > +
> >  static int loadvm_process_command_simple_lencheck(const char *name,
> >                                                    unsigned int actual,
> >                                                    unsigned int expected)
> > @@ -967,6 +977,8 @@ static int loadvm_process_command_simple_lencheck(const char *name,
> >  /*
> >   * Process an incoming 'QEMU_VM_COMMAND'
> >   * negative return on error (will issue error message)
> > + * 0   just a normal return
> > + * 1   All good, but exit the loop
> 
> This should probably also mention the possibility of negative returns
> for errors.

'negative return on error' it says there; I've made that clearer now
so it now has a '<0'  line.

> Am I correct in thinking that at this point the function never returns
> 1?  I'm assuming later patches in the series change that.

Yes, it's one of the commands that now returns that flag.
(Actually it was the LOADVM_QUIT_* enum, not '1')

> Maybe I'm missing something in my mental model here, but tying the
> duration of the containing loop to execution of specific commands
> seems problematic.  What's the circumstance in which it makes sense
> for a command to indicate that the rest of the packaged data should be
> essentially ignored

Yes, the only case we actually care about here in the postcopy case
is that when we are reading from the package we stop the main loops
ever reading from the main fd, since the listen thread takes that over.

> >   */
> >  static int loadvm_process_command(QEMUFile *f)
> >  {
> > @@ -1036,36 +1048,13 @@ void loadvm_free_handlers(MigrationIncomingState *mis)
> >      }
> >  }
> >  
> > -int qemu_loadvm_state(QEMUFile *f)
> > +static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> >  {
> > -    MigrationIncomingState *mis = migration_incoming_get_current();
> > -    Error *local_err = NULL;
> >      uint8_t section_type;
> > -    unsigned int v;
> >      int ret;
> > +    int exitcode = 0;
> >  
> > -    if (qemu_savevm_state_blocked(&local_err)) {
> > -        error_report("%s", error_get_pretty(local_err));
> > -        error_free(local_err);
> > -        return -EINVAL;
> > -    }
> > -
> > -    v = qemu_get_be32(f);
> > -    if (v != QEMU_VM_FILE_MAGIC) {
> > -        error_report("Not a migration stream");
> > -        return -EINVAL;
> > -    }
> > -
> > -    v = qemu_get_be32(f);
> > -    if (v == QEMU_VM_FILE_VERSION_COMPAT) {
> > -        error_report("SaveVM v2 format is obsolete and don't work anymore");
> > -        return -ENOTSUP;
> > -    }
> > -    if (v != QEMU_VM_FILE_VERSION) {
> > -        error_report("Unsupported migration stream version");
> > -        return -ENOTSUP;
> > -    }
> > -
> > +    trace_qemu_loadvm_state_main();
> >      while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
> >          uint32_t instance_id, version_id, section_id;
> >          SaveStateEntry *se;
> > @@ -1093,16 +1082,14 @@ int qemu_loadvm_state(QEMUFile *f)
> >              if (se == NULL) {
> >                  error_report("Unknown savevm section or instance '%s' %d",
> >                               idstr, instance_id);
> > -                ret = -EINVAL;
> > -                goto out;
> > +                return -EINVAL;
> >              }
> >  
> >              /* Validate version */
> >              if (version_id > se->version_id) {
> >                  error_report("savevm: unsupported version %d for '%s' v%d",
> >                               version_id, idstr, se->version_id);
> > -                ret = -EINVAL;
> > -                goto out;
> > +                return -EINVAL;
> >              }
> >  
> >              /* Add entry */
> > @@ -1117,7 +1104,7 @@ int qemu_loadvm_state(QEMUFile *f)
> >              if (ret < 0) {
> >                  error_report("error while loading state for instance 0x%x of"
> >                               " device '%s'", instance_id, idstr);
> > -                goto out;
> > +                return ret;
> >              }
> >              break;
> >          case QEMU_VM_SECTION_PART:
> > @@ -1132,36 +1119,73 @@ int qemu_loadvm_state(QEMUFile *f)
> >              }
> >              if (le == NULL) {
> >                  error_report("Unknown savevm section %d", section_id);
> > -                ret = -EINVAL;
> > -                goto out;
> > +                return -EINVAL;
> >              }
> >  
> >              ret = vmstate_load(f, le->se, le->version_id);
> >              if (ret < 0) {
> >                  error_report("error while loading state section id %d(%s)",
> >                               section_id, le->se->idstr);
> > -                goto out;
> > +                return ret;
> >              }
> >              break;
> >          case QEMU_VM_COMMAND:
> >              ret = loadvm_process_command(f);
> > -            if (ret < 0) {
> > -                goto out;
> > +            trace_qemu_loadvm_state_section_command(ret);
> > +            if ((ret < 0) || (ret & LOADVM_QUIT_LOOP)) {
> > +                return ret;
> >              }
> > +            exitcode |= ret; /* Lets us pass flags up to the parent */
> >              break;
> >          default:
> >              error_report("Unknown savevm section type %d", section_type);
> > -            ret = -EINVAL;
> > -            goto out;
> > +            return -EINVAL;
> >          }
> >      }
> >  
> > -    cpu_synchronize_all_post_init();
> > +    if (exitcode & LOADVM_QUIT_PARENT) {
> > +        trace_qemu_loadvm_state_main_quit_parent();
> > +        exitcode &= ~LOADVM_QUIT_PARENT;
> > +        exitcode |= LOADVM_QUIT_LOOP;
> > +    }
> 
> So, if I'm following properly putting a QUIT_PARENT will cause this
> loop to exit, also returning QUIT_LOOP, so the next loop out also
> quits.  If there was a third lood beyond that it wouldn't quit.
> 
> But are those really the semantics you want; or do you want the
> options to be "quit one level" and "quit all levels", which seems a
> little bit simpler.  In the current plans you only have the two levels
> so they're equivalent.

Right, that's all gone - now have a 'quit all levels..

Dave

> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK