| Message ID | 20171110203516.17027-1-danielhb@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 10, 2017 at 06:35:16PM -0200, Daniel Henrique Barboza wrote: > When migrating a VM with 'migrate_set_capability postcopy-ram on' > a postcopy_state is set during the process, ending up with the > state POSTCOPY_INCOMING_END when the migration is over. This > postcopy_state is taken into account inside ram_load to check > how it will load the memory pages. This same ram_load is called when > in a loadvm command. > > Inside ram_load, the logic to see if we're at postcopy_running state > is: > > postcopy_running = postcopy_state_get() >= POSTCOPY_INCOMING_LISTENING > > postcopy_state_get() returns this enum type: > > typedef enum { > POSTCOPY_INCOMING_NONE = 0, > POSTCOPY_INCOMING_ADVISE, > POSTCOPY_INCOMING_DISCARD, > POSTCOPY_INCOMING_LISTENING, > POSTCOPY_INCOMING_RUNNING, > POSTCOPY_INCOMING_END > } PostcopyState; > > In the case where ram_load is executed and postcopy_state is > POSTCOPY_INCOMING_END, postcopy_running will be set to 'true' and > ram_load will behave like a postcopy is in progress. This scenario isn't > achievable in a migration but it is reproducible when executing > savevm/loadvm after migrating with 'postcopy-ram on', causing loadvm > to fail with Error -22: > > Source: > > (qemu) migrate_set_capability postcopy-ram on > (qemu) migrate tcp:127.0.0.1:4444 > > Dest: > > (qemu) migrate_set_capability postcopy-ram on > (qemu) > ubuntu1704-intel login: > Ubuntu 17.04 ubuntu1704-intel ttyS0 > > ubuntu1704-intel login: (qemu) > (qemu) savevm test1 > (qemu) loadvm test1 > Unknown combination of migration flags: 0x4 (postcopy mode) > error while loading state for instance 0x0 of device 'ram' > Error -22 while loading VM state > (qemu) > > This patch fixes this problem by changing a bit the semantics > of postcopy_running inside ram_load, verifying first if > we're not in the POSTCOPY_INCOMING_END state. In this case, > postcopy_running is set to 'false'. > > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > --- > migration/ram.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 8620aa400a..43ed719668 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -2803,13 +2803,21 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > int flags = 0, ret = 0, invalid_flags = 0; > static uint64_t seq_iter; > int len = 0; > - /* > - * If system is running in postcopy mode, page inserts to host memory must > - * be atomic > - */ > - bool postcopy_running = postcopy_state_get() >= POSTCOPY_INCOMING_LISTENING; > - /* ADVISE is earlier, it shows the source has the postcopy capability on */ > - bool postcopy_advised = postcopy_state_get() >= POSTCOPY_INCOMING_ADVISE; > + bool postcopy_advised = false, postcopy_running = false; > + uint8_t postcopy_state = postcopy_state_get(); > + > + if (postcopy_state != POSTCOPY_INCOMING_END) { > + /* > + * If system is running in postcopy mode, page inserts to host memory > + * must be atomic > + */ > + postcopy_running = postcopy_state >= POSTCOPY_INCOMING_LISTENING; > + > + /* ADVISE is earlier, it shows the source has the postcopy > + * capability on > + */ > + postcopy_advised = postcopy_state >= POSTCOPY_INCOMING_ADVISE; > + } For me, this is not that clear. I would prefer something simpler like: PostcopyState state = postcopy_state_get(); bool postcopy_running = state >= POSTCOPY_INCOMING_LISTENING && \ state < POSTCOPY_INCOMING_END; Or we can introduce postcopy_is_running() helper. Same thing to postcopy_advised. Thanks, > > seq_iter++; > > -- > 2.13.6 > >
Hi Peter, On 11/13/2017 01:22 AM, Peter Xu wrote: > On Fri, Nov 10, 2017 at 06:35:16PM -0200, Daniel Henrique Barboza wrote: >> When migrating a VM with 'migrate_set_capability postcopy-ram on' >> a postcopy_state is set during the process, ending up with the >> state POSTCOPY_INCOMING_END when the migration is over. This >> postcopy_state is taken into account inside ram_load to check >> how it will load the memory pages. This same ram_load is called when >> in a loadvm command. >> >> Inside ram_load, the logic to see if we're at postcopy_running state >> is: >> >> postcopy_running = postcopy_state_get() >= POSTCOPY_INCOMING_LISTENING >> >> postcopy_state_get() returns this enum type: >> >> typedef enum { >> POSTCOPY_INCOMING_NONE = 0, >> POSTCOPY_INCOMING_ADVISE, >> POSTCOPY_INCOMING_DISCARD, >> POSTCOPY_INCOMING_LISTENING, >> POSTCOPY_INCOMING_RUNNING, >> POSTCOPY_INCOMING_END >> } PostcopyState; >> >> In the case where ram_load is executed and postcopy_state is >> POSTCOPY_INCOMING_END, postcopy_running will be set to 'true' and >> ram_load will behave like a postcopy is in progress. This scenario isn't >> achievable in a migration but it is reproducible when executing >> savevm/loadvm after migrating with 'postcopy-ram on', causing loadvm >> to fail with Error -22: >> >> Source: >> >> (qemu) migrate_set_capability postcopy-ram on >> (qemu) migrate tcp:127.0.0.1:4444 >> >> Dest: >> >> (qemu) migrate_set_capability postcopy-ram on >> (qemu) >> ubuntu1704-intel login: >> Ubuntu 17.04 ubuntu1704-intel ttyS0 >> >> ubuntu1704-intel login: (qemu) >> (qemu) savevm test1 >> (qemu) loadvm test1 >> Unknown combination of migration flags: 0x4 (postcopy mode) >> error while loading state for instance 0x0 of device 'ram' >> Error -22 while loading VM state >> (qemu) >> >> This patch fixes this problem by changing a bit the semantics >> of postcopy_running inside ram_load, verifying first if >> we're not in the POSTCOPY_INCOMING_END state. In this case, >> postcopy_running is set to 'false'. >> >> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> >> --- >> migration/ram.c | 22 +++++++++++++++------- >> 1 file changed, 15 insertions(+), 7 deletions(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 8620aa400a..43ed719668 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -2803,13 +2803,21 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) >> int flags = 0, ret = 0, invalid_flags = 0; >> static uint64_t seq_iter; >> int len = 0; >> - /* >> - * If system is running in postcopy mode, page inserts to host memory must >> - * be atomic >> - */ >> - bool postcopy_running = postcopy_state_get() >= POSTCOPY_INCOMING_LISTENING; >> - /* ADVISE is earlier, it shows the source has the postcopy capability on */ >> - bool postcopy_advised = postcopy_state_get() >= POSTCOPY_INCOMING_ADVISE; >> + bool postcopy_advised = false, postcopy_running = false; >> + uint8_t postcopy_state = postcopy_state_get(); >> + >> + if (postcopy_state != POSTCOPY_INCOMING_END) { >> + /* >> + * If system is running in postcopy mode, page inserts to host memory >> + * must be atomic >> + */ >> + postcopy_running = postcopy_state >= POSTCOPY_INCOMING_LISTENING; >> + >> + /* ADVISE is earlier, it shows the source has the postcopy >> + * capability on >> + */ >> + postcopy_advised = postcopy_state >= POSTCOPY_INCOMING_ADVISE; >> + } > For me, this is not that clear. I would prefer something simpler like: > > PostcopyState state = postcopy_state_get(); > bool postcopy_running = state >= POSTCOPY_INCOMING_LISTENING && \ > state < POSTCOPY_INCOMING_END; > > Or we can introduce postcopy_is_running() helper. Same thing to > postcopy_advised. Thanks, I like the idea of creating helpers to make the logic simpler. I'll make this change in the next spin. Thanks, Daniel > >> >> seq_iter++; >> >> -- >> 2.13.6 >> >>
Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> wrote: > When migrating a VM with 'migrate_set_capability postcopy-ram on' > a postcopy_state is set during the process, ending up with the > state POSTCOPY_INCOMING_END when the migration is over. This > postcopy_state is taken into account inside ram_load to check > how it will load the memory pages. This same ram_load is called when > in a loadvm command. > > Inside ram_load, the logic to see if we're at postcopy_running state > is: > > postcopy_running = postcopy_state_get() >= POSTCOPY_INCOMING_LISTENING > > postcopy_state_get() returns this enum type: > > typedef enum { > POSTCOPY_INCOMING_NONE = 0, > POSTCOPY_INCOMING_ADVISE, > POSTCOPY_INCOMING_DISCARD, > POSTCOPY_INCOMING_LISTENING, > POSTCOPY_INCOMING_RUNNING, > POSTCOPY_INCOMING_END > } PostcopyState; > > In the case where ram_load is executed and postcopy_state is > POSTCOPY_INCOMING_END, postcopy_running will be set to 'true' and > ram_load will behave like a postcopy is in progress. This scenario isn't > achievable in a migration but it is reproducible when executing > savevm/loadvm after migrating with 'postcopy-ram on', causing loadvm > to fail with Error -22: > > Source: > > (qemu) migrate_set_capability postcopy-ram on > (qemu) migrate tcp:127.0.0.1:4444 > > Dest: > > (qemu) migrate_set_capability postcopy-ram on > (qemu) > ubuntu1704-intel login: > Ubuntu 17.04 ubuntu1704-intel ttyS0 > > ubuntu1704-intel login: (qemu) > (qemu) savevm test1 > (qemu) loadvm test1 > Unknown combination of migration flags: 0x4 (postcopy mode) > error while loading state for instance 0x0 of device 'ram' > Error -22 while loading VM state > (qemu) > > This patch fixes this problem by changing a bit the semantics > of postcopy_running inside ram_load, verifying first if > we're not in the POSTCOPY_INCOMING_END state. In this case, > postcopy_running is set to 'false'. > > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> Reviewed-by: Juan Quintela <quintela@redhat.com> queued > --- > migration/ram.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 8620aa400a..43ed719668 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -2803,13 +2803,21 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > int flags = 0, ret = 0, invalid_flags = 0; > static uint64_t seq_iter; > int len = 0; > - /* > - * If system is running in postcopy mode, page inserts to host memory must > - * be atomic > - */ > - bool postcopy_running = postcopy_state_get() >= POSTCOPY_INCOMING_LISTENING; > - /* ADVISE is earlier, it shows the source has the postcopy capability on */ > - bool postcopy_advised = postcopy_state_get() >= POSTCOPY_INCOMING_ADVISE; > + bool postcopy_advised = false, postcopy_running = false; > + uint8_t postcopy_state = postcopy_state_get(); > + > + if (postcopy_state != POSTCOPY_INCOMING_END) { > + /* > + * If system is running in postcopy mode, page inserts to host memory > + * must be atomic > + */ > + postcopy_running = postcopy_state >= POSTCOPY_INCOMING_LISTENING; > + > + /* ADVISE is earlier, it shows the source has the postcopy > + * capability on > + */ > + postcopy_advised = postcopy_state >= POSTCOPY_INCOMING_ADVISE; > + } > > seq_iter++;
* Juan Quintela (quintela@redhat.com) wrote: > Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> wrote: > > When migrating a VM with 'migrate_set_capability postcopy-ram on' > > a postcopy_state is set during the process, ending up with the > > state POSTCOPY_INCOMING_END when the migration is over. This > > postcopy_state is taken into account inside ram_load to check > > how it will load the memory pages. This same ram_load is called when > > in a loadvm command. > > > > Inside ram_load, the logic to see if we're at postcopy_running state > > is: > > > > postcopy_running = postcopy_state_get() >= POSTCOPY_INCOMING_LISTENING > > > > postcopy_state_get() returns this enum type: > > > > typedef enum { > > POSTCOPY_INCOMING_NONE = 0, > > POSTCOPY_INCOMING_ADVISE, > > POSTCOPY_INCOMING_DISCARD, > > POSTCOPY_INCOMING_LISTENING, > > POSTCOPY_INCOMING_RUNNING, > > POSTCOPY_INCOMING_END > > } PostcopyState; > > > > In the case where ram_load is executed and postcopy_state is > > POSTCOPY_INCOMING_END, postcopy_running will be set to 'true' and > > ram_load will behave like a postcopy is in progress. This scenario isn't > > achievable in a migration but it is reproducible when executing > > savevm/loadvm after migrating with 'postcopy-ram on', causing loadvm > > to fail with Error -22: > > > > Source: > > > > (qemu) migrate_set_capability postcopy-ram on > > (qemu) migrate tcp:127.0.0.1:4444 > > > > Dest: > > > > (qemu) migrate_set_capability postcopy-ram on > > (qemu) > > ubuntu1704-intel login: > > Ubuntu 17.04 ubuntu1704-intel ttyS0 > > > > ubuntu1704-intel login: (qemu) > > (qemu) savevm test1 > > (qemu) loadvm test1 > > Unknown combination of migration flags: 0x4 (postcopy mode) > > error while loading state for instance 0x0 of device 'ram' > > Error -22 while loading VM state > > (qemu) > > > > This patch fixes this problem by changing a bit the semantics > > of postcopy_running inside ram_load, verifying first if > > we're not in the POSTCOPY_INCOMING_END state. In this case, > > postcopy_running is set to 'false'. > > > > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > queued Wrong version; v3 is: http://lists.nongnu.org/archive/html/qemu-devel/2017-11/msg03188.html Dave > > --- > > migration/ram.c | 22 +++++++++++++++------- > > 1 file changed, 15 insertions(+), 7 deletions(-) > > > > diff --git a/migration/ram.c b/migration/ram.c > > index 8620aa400a..43ed719668 100644 > > --- a/migration/ram.c > > +++ b/migration/ram.c > > @@ -2803,13 +2803,21 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > > int flags = 0, ret = 0, invalid_flags = 0; > > static uint64_t seq_iter; > > int len = 0; > > - /* > > - * If system is running in postcopy mode, page inserts to host memory must > > - * be atomic > > - */ > > - bool postcopy_running = postcopy_state_get() >= POSTCOPY_INCOMING_LISTENING; > > - /* ADVISE is earlier, it shows the source has the postcopy capability on */ > > - bool postcopy_advised = postcopy_state_get() >= POSTCOPY_INCOMING_ADVISE; > > + bool postcopy_advised = false, postcopy_running = false; > > + uint8_t postcopy_state = postcopy_state_get(); > > + > > + if (postcopy_state != POSTCOPY_INCOMING_END) { > > + /* > > + * If system is running in postcopy mode, page inserts to host memory > > + * must be atomic > > + */ > > + postcopy_running = postcopy_state >= POSTCOPY_INCOMING_LISTENING; > > + > > + /* ADVISE is earlier, it shows the source has the postcopy > > + * capability on > > + */ > > + postcopy_advised = postcopy_state >= POSTCOPY_INCOMING_ADVISE; > > + } > > > > seq_iter++; -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> wrote: >> > When migrating a VM with 'migrate_set_capability postcopy-ram on' >> > a postcopy_state is set during the process, ending up with the >> > state POSTCOPY_INCOMING_END when the migration is over. This >> > postcopy_state is taken into account inside ram_load to check >> > how it will load the memory pages. This same ram_load is called when >> > in a loadvm command. >> > >> > Inside ram_load, the logic to see if we're at postcopy_running state >> > is: >> > >> > postcopy_running = postcopy_state_get() >= POSTCOPY_INCOMING_LISTENING >> > >> > postcopy_state_get() returns this enum type: >> > >> > typedef enum { >> > POSTCOPY_INCOMING_NONE = 0, >> > POSTCOPY_INCOMING_ADVISE, >> > POSTCOPY_INCOMING_DISCARD, >> > POSTCOPY_INCOMING_LISTENING, >> > POSTCOPY_INCOMING_RUNNING, >> > POSTCOPY_INCOMING_END >> > } PostcopyState; >> > >> > In the case where ram_load is executed and postcopy_state is >> > POSTCOPY_INCOMING_END, postcopy_running will be set to 'true' and >> > ram_load will behave like a postcopy is in progress. This scenario isn't >> > achievable in a migration but it is reproducible when executing >> > savevm/loadvm after migrating with 'postcopy-ram on', causing loadvm >> > to fail with Error -22: >> > >> > Source: >> > >> > (qemu) migrate_set_capability postcopy-ram on >> > (qemu) migrate tcp:127.0.0.1:4444 >> > >> > Dest: >> > >> > (qemu) migrate_set_capability postcopy-ram on >> > (qemu) >> > ubuntu1704-intel login: >> > Ubuntu 17.04 ubuntu1704-intel ttyS0 >> > >> > ubuntu1704-intel login: (qemu) >> > (qemu) savevm test1 >> > (qemu) loadvm test1 >> > Unknown combination of migration flags: 0x4 (postcopy mode) >> > error while loading state for instance 0x0 of device 'ram' >> > Error -22 while loading VM state >> > (qemu) >> > >> > This patch fixes this problem by changing a bit the semantics >> > of postcopy_running inside ram_load, verifying first if >> > we're not in the POSTCOPY_INCOMING_END state. In this case, >> > postcopy_running is set to 'false'. >> > >> > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> >> >> Reviewed-by: Juan Quintela <quintela@redhat.com> >> >> queued > > Wrong version; v3 is: My bad, sorry. Got a new one. No clue why search on my INBOX didn't found this version. compiling a new pull request. > > http://lists.nongnu.org/archive/html/qemu-devel/2017-11/msg03188.html > > Dave > >> > --- >> > migration/ram.c | 22 +++++++++++++++------- >> > 1 file changed, 15 insertions(+), 7 deletions(-) >> > >> > diff --git a/migration/ram.c b/migration/ram.c >> > index 8620aa400a..43ed719668 100644 >> > --- a/migration/ram.c >> > +++ b/migration/ram.c >> > @@ -2803,13 +2803,21 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) >> > int flags = 0, ret = 0, invalid_flags = 0; >> > static uint64_t seq_iter; >> > int len = 0; >> > - /* >> > - * If system is running in postcopy mode, page inserts to host memory must >> > - * be atomic >> > - */ >> > - bool postcopy_running = postcopy_state_get() >= POSTCOPY_INCOMING_LISTENING; >> > - /* ADVISE is earlier, it shows the source has the postcopy capability on */ >> > - bool postcopy_advised = postcopy_state_get() >= POSTCOPY_INCOMING_ADVISE; >> > + bool postcopy_advised = false, postcopy_running = false; >> > + uint8_t postcopy_state = postcopy_state_get(); >> > + >> > + if (postcopy_state != POSTCOPY_INCOMING_END) { >> > + /* >> > + * If system is running in postcopy mode, page inserts to host memory >> > + * must be atomic >> > + */ >> > + postcopy_running = postcopy_state >= POSTCOPY_INCOMING_LISTENING; >> > + >> > + /* ADVISE is earlier, it shows the source has the postcopy >> > + * capability on >> > + */ >> > + postcopy_advised = postcopy_state >= POSTCOPY_INCOMING_ADVISE; >> > + } >> > >> > seq_iter++; > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/migration/ram.c b/migration/ram.c index 8620aa400a..43ed719668 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -2803,13 +2803,21 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) int flags = 0, ret = 0, invalid_flags = 0; static uint64_t seq_iter; int len = 0; - /* - * If system is running in postcopy mode, page inserts to host memory must - * be atomic - */ - bool postcopy_running = postcopy_state_get() >= POSTCOPY_INCOMING_LISTENING; - /* ADVISE is earlier, it shows the source has the postcopy capability on */ - bool postcopy_advised = postcopy_state_get() >= POSTCOPY_INCOMING_ADVISE; + bool postcopy_advised = false, postcopy_running = false; + uint8_t postcopy_state = postcopy_state_get(); + + if (postcopy_state != POSTCOPY_INCOMING_END) { + /* + * If system is running in postcopy mode, page inserts to host memory + * must be atomic + */ + postcopy_running = postcopy_state >= POSTCOPY_INCOMING_LISTENING; + + /* ADVISE is earlier, it shows the source has the postcopy + * capability on + */ + postcopy_advised = postcopy_state >= POSTCOPY_INCOMING_ADVISE; + } seq_iter++;
When migrating a VM with 'migrate_set_capability postcopy-ram on' a postcopy_state is set during the process, ending up with the state POSTCOPY_INCOMING_END when the migration is over. This postcopy_state is taken into account inside ram_load to check how it will load the memory pages. This same ram_load is called when in a loadvm command. Inside ram_load, the logic to see if we're at postcopy_running state is: postcopy_running = postcopy_state_get() >= POSTCOPY_INCOMING_LISTENING postcopy_state_get() returns this enum type: typedef enum { POSTCOPY_INCOMING_NONE = 0, POSTCOPY_INCOMING_ADVISE, POSTCOPY_INCOMING_DISCARD, POSTCOPY_INCOMING_LISTENING, POSTCOPY_INCOMING_RUNNING, POSTCOPY_INCOMING_END } PostcopyState; In the case where ram_load is executed and postcopy_state is POSTCOPY_INCOMING_END, postcopy_running will be set to 'true' and ram_load will behave like a postcopy is in progress. This scenario isn't achievable in a migration but it is reproducible when executing savevm/loadvm after migrating with 'postcopy-ram on', causing loadvm to fail with Error -22: Source: (qemu) migrate_set_capability postcopy-ram on (qemu) migrate tcp:127.0.0.1:4444 Dest: (qemu) migrate_set_capability postcopy-ram on (qemu) ubuntu1704-intel login: Ubuntu 17.04 ubuntu1704-intel ttyS0 ubuntu1704-intel login: (qemu) (qemu) savevm test1 (qemu) loadvm test1 Unknown combination of migration flags: 0x4 (postcopy mode) error while loading state for instance 0x0 of device 'ram' Error -22 while loading VM state (qemu) This patch fixes this problem by changing a bit the semantics of postcopy_running inside ram_load, verifying first if we're not in the POSTCOPY_INCOMING_END state. In this case, postcopy_running is set to 'false'. Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> --- migration/ram.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)