| Message ID | 1412358473-31398-20-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 03/10/2014 19:47, Dr. David Alan Gilbert (git) ha scritto: > > +/* These are ORable flags */ ... make them an "enum". > +const int LOADVM_EXITCODE_QUITLOOP = 1; > +const int LOADVM_EXITCODE_QUITPARENT = 2; LOADVM_QUIT_ALL, LOADVM_QUIT respectively? > +const int LOADVM_EXITCODE_KEEPHANDLERS = 4; > + Is it more common to drop or keep handlers? In either case, please add a comment to the three constants that details how to use them. In particular, please document why you should drop (resp. keep) handlers... Is it by chance that they are only used in savevm.c? Should they be moved to a header file? > > + if (exitcode & LOADVM_EXITCODE_QUITPARENT) { > + DPRINTF("loadvm_handlers_state_main: End of loop with QUITPARENT"); > + exitcode &= ~LOADVM_EXITCODE_QUITPARENT; > + exitcode &= LOADVM_EXITCODE_QUITLOOP; Either you want |=, or the first &= is useless. Paolo
* Paolo Bonzini (pbonzini@redhat.com) wrote: > Il 03/10/2014 19:47, Dr. David Alan Gilbert (git) ha scritto: > > > > +/* These are ORable flags */ > > ... make them an "enum". OK, will do - I'd generally tended to avoid using enum for things that were ORable where the combinations weren't themselves members of the enum; but I can do that. > > +const int LOADVM_EXITCODE_QUITLOOP = 1; > > +const int LOADVM_EXITCODE_QUITPARENT = 2; > > LOADVM_QUIT_ALL, LOADVM_QUIT respectively? > > +const int LOADVM_EXITCODE_KEEPHANDLERS = 4; > > + > > Is it more common to drop or keep handlers? I'ts more common to drop them. > In either case, please add a comment to the three constants that details > how to use them. In particular, please document why you should drop > (resp. keep) handlers... Does this make it clearer: /* 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, /* * Keep the LoadStateEntry handler list after the loop exits, * because they're being used in another thread. */ LOADVM_KEEP_HANDLERS = 4, }; > Is it by chance that they are only used in savevm.c? Should they be > moved to a header file? They're local. > > + if (exitcode & LOADVM_EXITCODE_QUITPARENT) { > > + DPRINTF("loadvm_handlers_state_main: End of loop with QUITPARENT"); > > + exitcode &= ~LOADVM_EXITCODE_QUITPARENT; > > + exitcode &= LOADVM_EXITCODE_QUITLOOP; > > Either you want |=, or the first &= is useless. Ooh nicely spotted; yes that should be |= - now I need to figure out why this didn't break things. The idea is we have: 1 outer loadvm_state loop 2 receives packaged command 3 inner_loadvm_state loop 4 receives handle_listen 5 < QUITPARENT 6 < QUITLOOP 7 < QUITLOOP 8 exits so QUITPARENT causes it's parent to exit, and to do that the inner loop transforms QUITPARENT into QUITLOOP as it's exit. Dave > > Paolo -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Il 07/10/2014 10:58, Dr. David Alan Gilbert ha scritto: > >>> > > + if (exitcode & LOADVM_EXITCODE_QUITPARENT) { >>> > > + DPRINTF("loadvm_handlers_state_main: End of loop with QUITPARENT"); >>> > > + exitcode &= ~LOADVM_EXITCODE_QUITPARENT; >>> > > + exitcode &= LOADVM_EXITCODE_QUITLOOP; >> > >> > Either you want |=, or the first &= is useless. > Ooh nicely spotted; yes that should be |= - now I need to figure out why this > didn't break things. > > The idea is we have: > 1 outer loadvm_state loop > 2 receives packaged command > 3 inner_loadvm_state loop > 4 receives handle_listen > 5 < QUITPARENT > 6 < QUITLOOP > 7 < QUITLOOP > 8 exits > > so QUITPARENT causes it's parent to exit, and to do that > the inner loop transforms QUITPARENT into QUITLOOP as it's > exit. Yes, that was my understanding as well. We have only two nested loops, but if we had three, should it be QUIT_PARENT or QUIT_ALL? Paolo
* Paolo Bonzini (pbonzini@redhat.com) wrote: > Il 07/10/2014 10:58, Dr. David Alan Gilbert ha scritto: > > > >>> > > + if (exitcode & LOADVM_EXITCODE_QUITPARENT) { > >>> > > + DPRINTF("loadvm_handlers_state_main: End of loop with QUITPARENT"); > >>> > > + exitcode &= ~LOADVM_EXITCODE_QUITPARENT; > >>> > > + exitcode &= LOADVM_EXITCODE_QUITLOOP; > >> > > >> > Either you want |=, or the first &= is useless. > > Ooh nicely spotted; yes that should be |= - now I need to figure out why this > > didn't break things. > > > > The idea is we have: > > 1 outer loadvm_state loop > > 2 receives packaged command > > 3 inner_loadvm_state loop > > 4 receives handle_listen > > 5 < QUITPARENT > > 6 < QUITLOOP > > 7 < QUITLOOP > > 8 exits > > > > so QUITPARENT causes it's parent to exit, and to do that > > the inner loop transforms QUITPARENT into QUITLOOP as it's > > exit. > > Yes, that was my understanding as well. > > We have only two nested loops, but if we had three, should it be > QUIT_PARENT or QUIT_ALL? The answer probably depends on why you've got 3 nested loops; either way is a bit of guesswork about what some potential future user wants to do. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Oct 03, 2014 at 06:47:25PM +0100, 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. > > loadvm_handlers is made static since it's lifetime is greater > than the outer qemu_loadvm_state. Maybe it's just me, but "made static" to me indicates either a change from fully-global to module-global, or (function) local automatic to local static, not a change from function local-automatic to module-global as here. It's also not clear from this patch alone why the lifetime of loadvm_handlers now needs to exceed that of qemu_loadvm_state().
* David Gibson (david@gibson.dropbear.id.au) wrote: > On Fri, Oct 03, 2014 at 06:47:25PM +0100, 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. > > > > loadvm_handlers is made static since it's lifetime is greater > > than the outer qemu_loadvm_state. > > Maybe it's just me, but "made static" to me indicates either a change > from fully-global to module-global, or (function) local automatic to > local static, not a change from function local-automatic to > module-global as here. > > It's also not clear from this patch alone why the lifetime of > loadvm_handlers now needs to exceed that of qemu_loadvm_state(). OK, how about if I reworked that last sentence to be: loadvm_handlers is made module-global to survive beyond the lifetime of the outer qemu_loadvm_state since it may still be in use by a subloop in the postcopy listen thread. 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
On Wed, Nov 19, 2014 at 05:50:11PM +0000, Dr. David Alan Gilbert wrote: > * David Gibson (david@gibson.dropbear.id.au) wrote: > > On Fri, Oct 03, 2014 at 06:47:25PM +0100, 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. > > > > > > loadvm_handlers is made static since it's lifetime is greater > > > than the outer qemu_loadvm_state. > > > > Maybe it's just me, but "made static" to me indicates either a change > > from fully-global to module-global, or (function) local automatic to > > local static, not a change from function local-automatic to > > module-global as here. > > > > It's also not clear from this patch alone why the lifetime of > > loadvm_handlers now needs to exceed that of qemu_loadvm_state(). > > OK, how about if I reworked that last sentence to be: > > loadvm_handlers is made module-global to survive beyond the lifetime > of the outer qemu_loadvm_state since it may still be in use by > a subloop in the postcopy listen thread. Yeah, that's better. A global seems ugly though. Would it be better to dynamically allocate the list head and pass a pointer into the listen thread, or even to pass the list head by value into the listen thread. The individual list elements need to be cleaned up at some point anyway, so I don't think that introduces any lifetime questions that weren't already there.
* David Gibson (david@gibson.dropbear.id.au) wrote: > On Wed, Nov 19, 2014 at 05:50:11PM +0000, Dr. David Alan Gilbert wrote: > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > On Fri, Oct 03, 2014 at 06:47:25PM +0100, 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. > > > > > > > > loadvm_handlers is made static since it's lifetime is greater > > > > than the outer qemu_loadvm_state. > > > > > > Maybe it's just me, but "made static" to me indicates either a change > > > from fully-global to module-global, or (function) local automatic to > > > local static, not a change from function local-automatic to > > > module-global as here. > > > > > > It's also not clear from this patch alone why the lifetime of > > > loadvm_handlers now needs to exceed that of qemu_loadvm_state(). > > > > OK, how about if I reworked that last sentence to be: > > > > loadvm_handlers is made module-global to survive beyond the lifetime > > of the outer qemu_loadvm_state since it may still be in use by > > a subloop in the postcopy listen thread. > > Yeah, that's better. A global seems ugly though. Would it be better > to dynamically allocate the list head and pass a pointer into the > listen thread, or even to pass the list head by value into the listen > thread. > > The individual list elements need to be cleaned up at some point > anyway, so I don't think that introduces any lifetime questions that > weren't already there. I've moved the loadvm_handlers out into the MigrationIncomingState structure, and free them when that is deallocated at the end of migration. 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
diff --git a/savevm.c b/savevm.c index 2c0d61a..7236232 100644 --- a/savevm.c +++ b/savevm.c @@ -915,6 +915,26 @@ static SaveStateEntry *find_se(const char *idstr, int instance_id) return NULL; } +/* These are ORable flags */ +const int LOADVM_EXITCODE_QUITLOOP = 1; +const int LOADVM_EXITCODE_QUITPARENT = 2; +const int LOADVM_EXITCODE_KEEPHANDLERS = 4; + +typedef struct LoadStateEntry { + QLIST_ENTRY(LoadStateEntry) entry; + SaveStateEntry *se; + int section_id; + int version_id; +} LoadStateEntry; + +typedef QLIST_HEAD(, LoadStateEntry) LoadStateEntry_Head; + +static LoadStateEntry_Head loadvm_handlers = + QLIST_HEAD_INITIALIZER(loadvm_handlers); + +static int qemu_loadvm_state_main(QEMUFile *f, + LoadStateEntry_Head *loadvm_handlers); + static int loadvm_process_command_simple_lencheck(const char *name, unsigned int actual, unsigned int expected) @@ -931,8 +951,11 @@ 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 */ -static int loadvm_process_command(QEMUFile *f) +static int loadvm_process_command(QEMUFile *f, + LoadStateEntry_Head *loadvm_handlers) { MigrationIncomingState *mis = migration_incoming_get_current(); uint16_t com; @@ -982,39 +1005,13 @@ static int loadvm_process_command(QEMUFile *f) return 0; } -typedef struct LoadStateEntry { - QLIST_ENTRY(LoadStateEntry) entry; - SaveStateEntry *se; - int section_id; - int version_id; -} LoadStateEntry; - -int qemu_loadvm_state(QEMUFile *f) +static int qemu_loadvm_state_main(QEMUFile *f, + LoadStateEntry_Head *loadvm_handlers) { - QLIST_HEAD(, LoadStateEntry) loadvm_handlers = - QLIST_HEAD_INITIALIZER(loadvm_handlers); - LoadStateEntry *le, *new_le; + LoadStateEntry *le; uint8_t section_type; - unsigned int v; int ret; - - if (qemu_savevm_state_blocked(NULL)) { - return -EINVAL; - } - - v = qemu_get_be32(f); - if (v != QEMU_VM_FILE_MAGIC) { - 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) { - return -ENOTSUP; - } + int exitcode = 0; while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) { uint32_t instance_id, version_id, section_id; @@ -1043,16 +1040,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 */ @@ -1061,14 +1056,14 @@ int qemu_loadvm_state(QEMUFile *f) le->se = se; le->section_id = section_id; le->version_id = version_id; - QLIST_INSERT_HEAD(&loadvm_handlers, le, entry); + QLIST_INSERT_HEAD(loadvm_handlers, le, entry); ret = vmstate_load(f, le->se, le->version_id); if (ret < 0) { error_report("qemu: error while loading state for" "instance 0x%x of device '%s'", instance_id, idstr); - goto out; + return ret; } break; case QEMU_VM_SECTION_PART: @@ -1076,47 +1071,84 @@ int qemu_loadvm_state(QEMUFile *f) section_id = qemu_get_be32(f); DPRINTF("QEMU_VM_SECTION_PART/END entry for id=%d", section_id); - QLIST_FOREACH(le, &loadvm_handlers, entry) { + QLIST_FOREACH(le, loadvm_handlers, entry) { if (le->section_id == section_id) { break; } } 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("qemu: error while loading state section" " id %d (%s)", section_id, le->se->idstr); - goto out; + return ret; } DPRINTF("QEMU_VM_SECTION_PART/END done for id=%d", section_id); break; case QEMU_VM_COMMAND: - ret = loadvm_process_command(f); - if (ret < 0) { - goto out; + ret = loadvm_process_command(f, loadvm_handlers); + DPRINTF("%s QEMU_VM_COMMAND ret: %d", __func__, ret); + if ((ret < 0) || (ret & LOADVM_EXITCODE_QUITLOOP)) { + 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; } } DPRINTF("qemu_loadvm_state loop: exited loop"); - cpu_synchronize_all_post_init(); + if (exitcode & LOADVM_EXITCODE_QUITPARENT) { + DPRINTF("loadvm_handlers_state_main: End of loop with QUITPARENT"); + exitcode &= ~LOADVM_EXITCODE_QUITPARENT; + exitcode &= LOADVM_EXITCODE_QUITLOOP; + } + + return exitcode; +} + +int qemu_loadvm_state(QEMUFile *f) +{ + LoadStateEntry *le, *new_le; + unsigned int v; + int ret; + + if (qemu_savevm_state_blocked(NULL)) { + return -EINVAL; + } + + v = qemu_get_be32(f); + if (v != QEMU_VM_FILE_MAGIC) { + return -EINVAL; + } - ret = 0; + 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) { + return -ENOTSUP; + } + + QLIST_INIT(&loadvm_handlers); + ret = qemu_loadvm_state_main(f, &loadvm_handlers); -out: - QLIST_FOREACH_SAFE(le, &loadvm_handlers, entry, new_le) { - QLIST_REMOVE(le, entry); - g_free(le); + if (ret == 0) { + cpu_synchronize_all_post_init(); + } + + if ((ret < 0) || !(ret & LOADVM_EXITCODE_KEEPHANDLERS)) { + QLIST_FOREACH_SAFE(le, &loadvm_handlers, entry, new_le) { + QLIST_REMOVE(le, entry); + g_free(le); + } } if (ret == 0) {