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

[David Gibson]

On Wed, Feb 25, 2015 at 04:51:38PM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
> 
> 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 <address@hidden>
> ---
>  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.

> +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.

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

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

>   */
>  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.
-- 
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

pgpy5f4ZNSZC_.pgp
Description: PGP signature