| Message ID | 1479837270-79005-4-git-send-email-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > Split common postcopy staff from ram postcopy staff. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/migration/migration.h | 1 + > migration/migration.c | 39 +++++++++++++++++++++++++++------------ > migration/postcopy-ram.c | 4 +++- > migration/savevm.c | 31 ++++++++++++++++++++++--------- > 4 files changed, 53 insertions(+), 22 deletions(-) > Hi { > MigrationState *s; > @@ -1587,9 +1592,11 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running) > * need to tell the destination to throw any pages it's already received > * that are dirty > */ > - if (ram_postcopy_send_discard_bitmap(ms)) { > - error_report("postcopy send discard bitmap failed"); > - goto fail; > + if (migrate_postcopy_ram()) { > + if (ram_postcopy_send_discard_bitmap(ms)) { > + error_report("postcopy send discard bitmap failed"); > + goto fail; > + } I will have preffered that for the ram commands, to embed the migrate_postocpy_ram() check inside them, but that is taste, and everyone has its own O:-) > diff --git a/migration/savevm.c b/migration/savevm.c > index e436cb2..c8a71c8 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -73,7 +73,7 @@ static struct mig_cmd_args { > [MIG_CMD_INVALID] = { .len = -1, .name = "INVALID" }, > [MIG_CMD_OPEN_RETURN_PATH] = { .len = 0, .name = "OPEN_RETURN_PATH" }, > [MIG_CMD_PING] = { .len = sizeof(uint32_t), .name = "PING" }, > - [MIG_CMD_POSTCOPY_ADVISE] = { .len = 16, .name = "POSTCOPY_ADVISE" }, > + [MIG_CMD_POSTCOPY_ADVISE] = { .len = -1, .name = "POSTCOPY_ADVISE" }, > [MIG_CMD_POSTCOPY_LISTEN] = { .len = 0, .name = "POSTCOPY_LISTEN" }, > [MIG_CMD_POSTCOPY_RUN] = { .len = 0, .name = "POSTCOPY_RUN" }, > [MIG_CMD_POSTCOPY_RAM_DISCARD] = { > @@ -827,12 +827,17 @@ int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len) > /* Send prior to any postcopy transfer */ > void qemu_savevm_send_postcopy_advise(QEMUFile *f) > { > - uint64_t tmp[2]; > - tmp[0] = cpu_to_be64(getpagesize()); > - tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); > + if (migrate_postcopy_ram()) { > + uint64_t tmp[2]; > + tmp[0] = cpu_to_be64(getpagesize()); > + tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); > > - trace_qemu_savevm_send_postcopy_advise(); > - qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 16, (uint8_t *)tmp); > + trace_qemu_savevm_send_postcopy_advise(); > + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, > + 16, (uint8_t *)tmp); > + } else { > + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 0, NULL); > + } > } > > /* Sent prior to starting the destination running in postcopy, discard pages I haven't yet figured out why you are reusing this command with a different number of parameters. For this to pass, I need that Dave comment on this. So, Reviewed-by: Juan Quintela <quintela@redhat.com> conditioned that Dave agrees with this.
24.01.2017 12:24, Juan Quintela wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >> Split common postcopy staff from ram postcopy staff. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> include/migration/migration.h | 1 + >> migration/migration.c | 39 +++++++++++++++++++++++++++------------ >> migration/postcopy-ram.c | 4 +++- >> migration/savevm.c | 31 ++++++++++++++++++++++--------- >> 4 files changed, 53 insertions(+), 22 deletions(-) >> > Hi > > { >> MigrationState *s; >> @@ -1587,9 +1592,11 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running) >> * need to tell the destination to throw any pages it's already received >> * that are dirty >> */ >> - if (ram_postcopy_send_discard_bitmap(ms)) { >> - error_report("postcopy send discard bitmap failed"); >> - goto fail; >> + if (migrate_postcopy_ram()) { >> + if (ram_postcopy_send_discard_bitmap(ms)) { >> + error_report("postcopy send discard bitmap failed"); >> + goto fail; >> + } > I will have preffered that for the ram commands, to embed the > migrate_postocpy_ram() check inside them, but that is taste, and > everyone has its own O:-) > >> diff --git a/migration/savevm.c b/migration/savevm.c >> index e436cb2..c8a71c8 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -73,7 +73,7 @@ static struct mig_cmd_args { >> [MIG_CMD_INVALID] = { .len = -1, .name = "INVALID" }, >> [MIG_CMD_OPEN_RETURN_PATH] = { .len = 0, .name = "OPEN_RETURN_PATH" }, >> [MIG_CMD_PING] = { .len = sizeof(uint32_t), .name = "PING" }, >> - [MIG_CMD_POSTCOPY_ADVISE] = { .len = 16, .name = "POSTCOPY_ADVISE" }, >> + [MIG_CMD_POSTCOPY_ADVISE] = { .len = -1, .name = "POSTCOPY_ADVISE" }, >> [MIG_CMD_POSTCOPY_LISTEN] = { .len = 0, .name = "POSTCOPY_LISTEN" }, >> [MIG_CMD_POSTCOPY_RUN] = { .len = 0, .name = "POSTCOPY_RUN" }, >> [MIG_CMD_POSTCOPY_RAM_DISCARD] = { >> @@ -827,12 +827,17 @@ int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len) >> /* Send prior to any postcopy transfer */ >> void qemu_savevm_send_postcopy_advise(QEMUFile *f) >> { >> - uint64_t tmp[2]; >> - tmp[0] = cpu_to_be64(getpagesize()); >> - tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); >> + if (migrate_postcopy_ram()) { >> + uint64_t tmp[2]; >> + tmp[0] = cpu_to_be64(getpagesize()); >> + tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); >> >> - trace_qemu_savevm_send_postcopy_advise(); >> - qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 16, (uint8_t *)tmp); >> + trace_qemu_savevm_send_postcopy_advise(); >> + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, >> + 16, (uint8_t *)tmp); >> + } else { >> + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 0, NULL); >> + } >> } >> >> /* Sent prior to starting the destination running in postcopy, discard pages > I haven't yet figured out why you are reusing this command with a > different number of parameters. > For this to pass, I need that Dave comment on this. > > So, > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > conditioned that Dave agrees with this. These parameters are unrelated if ram postcopy is disabled. So, I should be better to have a possibility of skipping them, then to send unneeded numbers and write separate code to read them from the stream (rewriting loadvm_postcopy_handle_advise to just read these two numbers and do nothing about ram postcopy for this case).
* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote: > 24.01.2017 12:24, Juan Quintela wrote: > > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > > > Split common postcopy staff from ram postcopy staff. > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > > --- > > > include/migration/migration.h | 1 + > > > migration/migration.c | 39 +++++++++++++++++++++++++++------------ > > > migration/postcopy-ram.c | 4 +++- > > > migration/savevm.c | 31 ++++++++++++++++++++++--------- > > > 4 files changed, 53 insertions(+), 22 deletions(-) > > > > > Hi > > > > { > > > MigrationState *s; > > > @@ -1587,9 +1592,11 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running) > > > * need to tell the destination to throw any pages it's already received > > > * that are dirty > > > */ > > > - if (ram_postcopy_send_discard_bitmap(ms)) { > > > - error_report("postcopy send discard bitmap failed"); > > > - goto fail; > > > + if (migrate_postcopy_ram()) { > > > + if (ram_postcopy_send_discard_bitmap(ms)) { > > > + error_report("postcopy send discard bitmap failed"); > > > + goto fail; > > > + } > > I will have preffered that for the ram commands, to embed the > > migrate_postocpy_ram() check inside them, but that is taste, and > > everyone has its own O:-) > > > > > diff --git a/migration/savevm.c b/migration/savevm.c > > > index e436cb2..c8a71c8 100644 > > > --- a/migration/savevm.c > > > +++ b/migration/savevm.c > > > @@ -73,7 +73,7 @@ static struct mig_cmd_args { > > > [MIG_CMD_INVALID] = { .len = -1, .name = "INVALID" }, > > > [MIG_CMD_OPEN_RETURN_PATH] = { .len = 0, .name = "OPEN_RETURN_PATH" }, > > > [MIG_CMD_PING] = { .len = sizeof(uint32_t), .name = "PING" }, > > > - [MIG_CMD_POSTCOPY_ADVISE] = { .len = 16, .name = "POSTCOPY_ADVISE" }, > > > + [MIG_CMD_POSTCOPY_ADVISE] = { .len = -1, .name = "POSTCOPY_ADVISE" }, > > > [MIG_CMD_POSTCOPY_LISTEN] = { .len = 0, .name = "POSTCOPY_LISTEN" }, > > > [MIG_CMD_POSTCOPY_RUN] = { .len = 0, .name = "POSTCOPY_RUN" }, > > > [MIG_CMD_POSTCOPY_RAM_DISCARD] = { > > > @@ -827,12 +827,17 @@ int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len) > > > /* Send prior to any postcopy transfer */ > > > void qemu_savevm_send_postcopy_advise(QEMUFile *f) > > > { > > > - uint64_t tmp[2]; > > > - tmp[0] = cpu_to_be64(getpagesize()); > > > - tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); > > > + if (migrate_postcopy_ram()) { > > > + uint64_t tmp[2]; > > > + tmp[0] = cpu_to_be64(getpagesize()); > > > + tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); > > > - trace_qemu_savevm_send_postcopy_advise(); > > > - qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 16, (uint8_t *)tmp); > > > + trace_qemu_savevm_send_postcopy_advise(); > > > + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, > > > + 16, (uint8_t *)tmp); > > > + } else { > > > + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 0, NULL); > > > + } > > > } > > > /* Sent prior to starting the destination running in postcopy, discard pages > > I haven't yet figured out why you are reusing this command with a > > different number of parameters. > > For this to pass, I need that Dave comment on this. > > > > So, > > > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > > > conditioned that Dave agrees with this. > > These parameters are unrelated if ram postcopy is disabled. So, I should be > better to have a possibility of skipping them, then to send unneeded numbers > and write separate code to read them from the stream (rewriting > loadvm_postcopy_handle_advise to just read these two numbers and do nothing > about ram postcopy for this case). I think I'd prefer either a new command or keeping these fields (probably all 0 ?) my worry is what happens in the case if we add a 3rd postcopy subfeature; In your case we have three possibilities: a) Postcopy RAM only - 16 bytes b) Postcopy persistent-dirty-bitmap only - 0 bytes c) Both - 16 bytes Lets say we added postcopy-foo in the future and it wanted to add another 16 bytes, what would it send if it was foo+persistent-dirty-bitmap and no RAM? We'd end up with 16 bytes sent but you'd have to be very careful never to get that confused with case (a) above. (I don't feel too strongly about it though) Dave > -- > Best regards, > Vladimir > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
24.01.2017 22:53, Dr. David Alan Gilbert wrote: > * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote: >> 24.01.2017 12:24, Juan Quintela wrote: >>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >>>> Split common postcopy staff from ram postcopy staff. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> include/migration/migration.h | 1 + >>>> migration/migration.c | 39 +++++++++++++++++++++++++++------------ >>>> migration/postcopy-ram.c | 4 +++- >>>> migration/savevm.c | 31 ++++++++++++++++++++++--------- >>>> 4 files changed, 53 insertions(+), 22 deletions(-) >>>> >>> Hi >>> >>> { >>>> MigrationState *s; >>>> @@ -1587,9 +1592,11 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running) >>>> * need to tell the destination to throw any pages it's already received >>>> * that are dirty >>>> */ >>>> - if (ram_postcopy_send_discard_bitmap(ms)) { >>>> - error_report("postcopy send discard bitmap failed"); >>>> - goto fail; >>>> + if (migrate_postcopy_ram()) { >>>> + if (ram_postcopy_send_discard_bitmap(ms)) { >>>> + error_report("postcopy send discard bitmap failed"); >>>> + goto fail; >>>> + } >>> I will have preffered that for the ram commands, to embed the >>> migrate_postocpy_ram() check inside them, but that is taste, and >>> everyone has its own O:-) >>> >>>> diff --git a/migration/savevm.c b/migration/savevm.c >>>> index e436cb2..c8a71c8 100644 >>>> --- a/migration/savevm.c >>>> +++ b/migration/savevm.c >>>> @@ -73,7 +73,7 @@ static struct mig_cmd_args { >>>> [MIG_CMD_INVALID] = { .len = -1, .name = "INVALID" }, >>>> [MIG_CMD_OPEN_RETURN_PATH] = { .len = 0, .name = "OPEN_RETURN_PATH" }, >>>> [MIG_CMD_PING] = { .len = sizeof(uint32_t), .name = "PING" }, >>>> - [MIG_CMD_POSTCOPY_ADVISE] = { .len = 16, .name = "POSTCOPY_ADVISE" }, >>>> + [MIG_CMD_POSTCOPY_ADVISE] = { .len = -1, .name = "POSTCOPY_ADVISE" }, >>>> [MIG_CMD_POSTCOPY_LISTEN] = { .len = 0, .name = "POSTCOPY_LISTEN" }, >>>> [MIG_CMD_POSTCOPY_RUN] = { .len = 0, .name = "POSTCOPY_RUN" }, >>>> [MIG_CMD_POSTCOPY_RAM_DISCARD] = { >>>> @@ -827,12 +827,17 @@ int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len) >>>> /* Send prior to any postcopy transfer */ >>>> void qemu_savevm_send_postcopy_advise(QEMUFile *f) >>>> { >>>> - uint64_t tmp[2]; >>>> - tmp[0] = cpu_to_be64(getpagesize()); >>>> - tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); >>>> + if (migrate_postcopy_ram()) { >>>> + uint64_t tmp[2]; >>>> + tmp[0] = cpu_to_be64(getpagesize()); >>>> + tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); >>>> - trace_qemu_savevm_send_postcopy_advise(); >>>> - qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 16, (uint8_t *)tmp); >>>> + trace_qemu_savevm_send_postcopy_advise(); >>>> + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, >>>> + 16, (uint8_t *)tmp); >>>> + } else { >>>> + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 0, NULL); >>>> + } >>>> } >>>> /* Sent prior to starting the destination running in postcopy, discard pages >>> I haven't yet figured out why you are reusing this command with a >>> different number of parameters. >>> For this to pass, I need that Dave comment on this. >>> >>> So, >>> >>> Reviewed-by: Juan Quintela <quintela@redhat.com> >>> >>> conditioned that Dave agrees with this. >> These parameters are unrelated if ram postcopy is disabled. So, I should be >> better to have a possibility of skipping them, then to send unneeded numbers >> and write separate code to read them from the stream (rewriting >> loadvm_postcopy_handle_advise to just read these two numbers and do nothing >> about ram postcopy for this case). > I think I'd prefer either a new command or keeping these fields (probably all 0 ?) > my worry is what happens in the case if we add a 3rd postcopy subfeature; > In your case we have three possibilities: > > a) Postcopy RAM only - 16 bytes > b) Postcopy persistent-dirty-bitmap only - 0 bytes > c) Both - 16 bytes > > Lets say we added postcopy-foo in the future and it wanted to add > another 16 bytes, what would it send if it was foo+persistent-dirty-bitmap and no RAM? > We'd end up with 16 bytes sent but you'd have to be very careful > never to get that confused with case (a) above. > > (I don't feel too strongly about it though) Hmm.. Actually I use migrate_postcopy_ram() to distinct these thing in loadvm_postcopy_handle_advise.. And it seem like it is a mistake. Are migration capabilities available on target?.. On the other hand postcopy-test doesn't fail... > > Dave > >> -- >> Best regards, >> Vladimir >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote: > 24.01.2017 22:53, Dr. David Alan Gilbert wrote: > > * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote: > > > 24.01.2017 12:24, Juan Quintela wrote: > > > > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > > > > > Split common postcopy staff from ram postcopy staff. > > > > > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > > > > --- > > > > > include/migration/migration.h | 1 + > > > > > migration/migration.c | 39 +++++++++++++++++++++++++++------------ > > > > > migration/postcopy-ram.c | 4 +++- > > > > > migration/savevm.c | 31 ++++++++++++++++++++++--------- > > > > > 4 files changed, 53 insertions(+), 22 deletions(-) > > > > > > > > > Hi > > > > > > > > { > > > > > MigrationState *s; > > > > > @@ -1587,9 +1592,11 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running) > > > > > * need to tell the destination to throw any pages it's already received > > > > > * that are dirty > > > > > */ > > > > > - if (ram_postcopy_send_discard_bitmap(ms)) { > > > > > - error_report("postcopy send discard bitmap failed"); > > > > > - goto fail; > > > > > + if (migrate_postcopy_ram()) { > > > > > + if (ram_postcopy_send_discard_bitmap(ms)) { > > > > > + error_report("postcopy send discard bitmap failed"); > > > > > + goto fail; > > > > > + } > > > > I will have preffered that for the ram commands, to embed the > > > > migrate_postocpy_ram() check inside them, but that is taste, and > > > > everyone has its own O:-) > > > > > > > > > diff --git a/migration/savevm.c b/migration/savevm.c > > > > > index e436cb2..c8a71c8 100644 > > > > > --- a/migration/savevm.c > > > > > +++ b/migration/savevm.c > > > > > @@ -73,7 +73,7 @@ static struct mig_cmd_args { > > > > > [MIG_CMD_INVALID] = { .len = -1, .name = "INVALID" }, > > > > > [MIG_CMD_OPEN_RETURN_PATH] = { .len = 0, .name = "OPEN_RETURN_PATH" }, > > > > > [MIG_CMD_PING] = { .len = sizeof(uint32_t), .name = "PING" }, > > > > > - [MIG_CMD_POSTCOPY_ADVISE] = { .len = 16, .name = "POSTCOPY_ADVISE" }, > > > > > + [MIG_CMD_POSTCOPY_ADVISE] = { .len = -1, .name = "POSTCOPY_ADVISE" }, > > > > > [MIG_CMD_POSTCOPY_LISTEN] = { .len = 0, .name = "POSTCOPY_LISTEN" }, > > > > > [MIG_CMD_POSTCOPY_RUN] = { .len = 0, .name = "POSTCOPY_RUN" }, > > > > > [MIG_CMD_POSTCOPY_RAM_DISCARD] = { > > > > > @@ -827,12 +827,17 @@ int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len) > > > > > /* Send prior to any postcopy transfer */ > > > > > void qemu_savevm_send_postcopy_advise(QEMUFile *f) > > > > > { > > > > > - uint64_t tmp[2]; > > > > > - tmp[0] = cpu_to_be64(getpagesize()); > > > > > - tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); > > > > > + if (migrate_postcopy_ram()) { > > > > > + uint64_t tmp[2]; > > > > > + tmp[0] = cpu_to_be64(getpagesize()); > > > > > + tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); > > > > > - trace_qemu_savevm_send_postcopy_advise(); > > > > > - qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 16, (uint8_t *)tmp); > > > > > + trace_qemu_savevm_send_postcopy_advise(); > > > > > + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, > > > > > + 16, (uint8_t *)tmp); > > > > > + } else { > > > > > + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 0, NULL); > > > > > + } > > > > > } > > > > > /* Sent prior to starting the destination running in postcopy, discard pages > > > > I haven't yet figured out why you are reusing this command with a > > > > different number of parameters. > > > > For this to pass, I need that Dave comment on this. > > > > > > > > So, > > > > > > > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > > > > > > > conditioned that Dave agrees with this. > > > These parameters are unrelated if ram postcopy is disabled. So, I should be > > > better to have a possibility of skipping them, then to send unneeded numbers > > > and write separate code to read them from the stream (rewriting > > > loadvm_postcopy_handle_advise to just read these two numbers and do nothing > > > about ram postcopy for this case). > > I think I'd prefer either a new command or keeping these fields (probably all 0 ?) > > my worry is what happens in the case if we add a 3rd postcopy subfeature; > > In your case we have three possibilities: > > > > a) Postcopy RAM only - 16 bytes > > b) Postcopy persistent-dirty-bitmap only - 0 bytes > > c) Both - 16 bytes > > > > Lets say we added postcopy-foo in the future and it wanted to add > > another 16 bytes, what would it send if it was foo+persistent-dirty-bitmap and no RAM? > > We'd end up with 16 bytes sent but you'd have to be very careful > > never to get that confused with case (a) above. > > > > (I don't feel too strongly about it though) > > Hmm.. Actually I use migrate_postcopy_ram() to distinct these thing in > loadvm_postcopy_handle_advise.. And it seem like it is a mistake. Are > migration capabilities available on target?.. On the other hand > postcopy-test doesn't fail... Libvirt does set it on the destination, and it's already useful for checking the destination host has the appropriate kernel userfault support; so I'm fine with requiring it. However it's good where possible to fail nicely if someone doesn't set it. Dave > > > > Dave > > > > > -- > > > Best regards, > > > Vladimir > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > -- > Best regards, > Vladimir > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
24.01.2017 22:53, Dr. David Alan Gilbert wrote: > * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote: >> 24.01.2017 12:24, Juan Quintela wrote: >>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >>>> Split common postcopy staff from ram postcopy staff. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> include/migration/migration.h | 1 + >>>> migration/migration.c | 39 +++++++++++++++++++++++++++------------ >>>> migration/postcopy-ram.c | 4 +++- >>>> migration/savevm.c | 31 ++++++++++++++++++++++--------- >>>> 4 files changed, 53 insertions(+), 22 deletions(-) >>>> >>> Hi >>> >>> { >>>> MigrationState *s; >>>> @@ -1587,9 +1592,11 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running) >>>> * need to tell the destination to throw any pages it's already received >>>> * that are dirty >>>> */ >>>> - if (ram_postcopy_send_discard_bitmap(ms)) { >>>> - error_report("postcopy send discard bitmap failed"); >>>> - goto fail; >>>> + if (migrate_postcopy_ram()) { >>>> + if (ram_postcopy_send_discard_bitmap(ms)) { >>>> + error_report("postcopy send discard bitmap failed"); >>>> + goto fail; >>>> + } >>> I will have preffered that for the ram commands, to embed the >>> migrate_postocpy_ram() check inside them, but that is taste, and >>> everyone has its own O:-) >>> >>>> diff --git a/migration/savevm.c b/migration/savevm.c >>>> index e436cb2..c8a71c8 100644 >>>> --- a/migration/savevm.c >>>> +++ b/migration/savevm.c >>>> @@ -73,7 +73,7 @@ static struct mig_cmd_args { >>>> [MIG_CMD_INVALID] = { .len = -1, .name = "INVALID" }, >>>> [MIG_CMD_OPEN_RETURN_PATH] = { .len = 0, .name = "OPEN_RETURN_PATH" }, >>>> [MIG_CMD_PING] = { .len = sizeof(uint32_t), .name = "PING" }, >>>> - [MIG_CMD_POSTCOPY_ADVISE] = { .len = 16, .name = "POSTCOPY_ADVISE" }, >>>> + [MIG_CMD_POSTCOPY_ADVISE] = { .len = -1, .name = "POSTCOPY_ADVISE" }, >>>> [MIG_CMD_POSTCOPY_LISTEN] = { .len = 0, .name = "POSTCOPY_LISTEN" }, >>>> [MIG_CMD_POSTCOPY_RUN] = { .len = 0, .name = "POSTCOPY_RUN" }, >>>> [MIG_CMD_POSTCOPY_RAM_DISCARD] = { >>>> @@ -827,12 +827,17 @@ int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len) >>>> /* Send prior to any postcopy transfer */ >>>> void qemu_savevm_send_postcopy_advise(QEMUFile *f) >>>> { >>>> - uint64_t tmp[2]; >>>> - tmp[0] = cpu_to_be64(getpagesize()); >>>> - tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); >>>> + if (migrate_postcopy_ram()) { >>>> + uint64_t tmp[2]; >>>> + tmp[0] = cpu_to_be64(getpagesize()); >>>> + tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); >>>> - trace_qemu_savevm_send_postcopy_advise(); >>>> - qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 16, (uint8_t *)tmp); >>>> + trace_qemu_savevm_send_postcopy_advise(); >>>> + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, >>>> + 16, (uint8_t *)tmp); >>>> + } else { >>>> + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 0, NULL); >>>> + } >>>> } >>>> /* Sent prior to starting the destination running in postcopy, discard pages >>> I haven't yet figured out why you are reusing this command with a >>> different number of parameters. >>> For this to pass, I need that Dave comment on this. >>> >>> So, >>> >>> Reviewed-by: Juan Quintela <quintela@redhat.com> >>> >>> conditioned that Dave agrees with this. >> These parameters are unrelated if ram postcopy is disabled. So, I should be >> better to have a possibility of skipping them, then to send unneeded numbers >> and write separate code to read them from the stream (rewriting >> loadvm_postcopy_handle_advise to just read these two numbers and do nothing >> about ram postcopy for this case). > I think I'd prefer either a new command or keeping these fields (probably all 0 ?) > my worry is what happens in the case if we add a 3rd postcopy subfeature; > In your case we have three possibilities: > > a) Postcopy RAM only - 16 bytes > b) Postcopy persistent-dirty-bitmap only - 0 bytes > c) Both - 16 bytes > > Lets say we added postcopy-foo in the future and it wanted to add > another 16 bytes, what would it send if it was foo+persistent-dirty-bitmap and no RAM? > We'd end up with 16 bytes sent but you'd have to be very careful > never to get that confused with case (a) above. > > (I don't feel too strongly about it though) We would like to stick current solution if it is not a problem. About postcopy-foo: on the one hand, the format may be determined from migration capabilities, on the other hand we can add for example 4bytes format identifier starting from postcopy-foo and make all parameters size to be at least 20 bytes. Or something like this. > > Dave > >> -- >> Best regards, >> Vladimir >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
01.02.2017 14:06, Vladimir Sementsov-Ogievskiy wrote: > 24.01.2017 22:53, Dr. David Alan Gilbert wrote: >> * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote: >>> 24.01.2017 12:24, Juan Quintela wrote: >>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >>>>> Split common postcopy staff from ram postcopy staff. >>>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>> <vsementsov@virtuozzo.com> >>>>> --- >>>>> include/migration/migration.h | 1 + >>>>> migration/migration.c | 39 >>>>> +++++++++++++++++++++++++++------------ >>>>> migration/postcopy-ram.c | 4 +++- >>>>> migration/savevm.c | 31 ++++++++++++++++++++++--------- >>>>> 4 files changed, 53 insertions(+), 22 deletions(-) >>>>> >>>> Hi >>>> >>>> { >>>>> MigrationState *s; >>>>> @@ -1587,9 +1592,11 @@ static int postcopy_start(MigrationState >>>>> *ms, bool *old_vm_running) >>>>> * need to tell the destination to throw any pages it's >>>>> already received >>>>> * that are dirty >>>>> */ >>>>> - if (ram_postcopy_send_discard_bitmap(ms)) { >>>>> - error_report("postcopy send discard bitmap failed"); >>>>> - goto fail; >>>>> + if (migrate_postcopy_ram()) { >>>>> + if (ram_postcopy_send_discard_bitmap(ms)) { >>>>> + error_report("postcopy send discard bitmap failed"); >>>>> + goto fail; >>>>> + } >>>> I will have preffered that for the ram commands, to embed the >>>> migrate_postocpy_ram() check inside them, but that is taste, and >>>> everyone has its own O:-) >>>> >>>>> diff --git a/migration/savevm.c b/migration/savevm.c >>>>> index e436cb2..c8a71c8 100644 >>>>> --- a/migration/savevm.c >>>>> +++ b/migration/savevm.c >>>>> @@ -73,7 +73,7 @@ static struct mig_cmd_args { >>>>> [MIG_CMD_INVALID] = { .len = -1, .name = "INVALID" }, >>>>> [MIG_CMD_OPEN_RETURN_PATH] = { .len = 0, .name = >>>>> "OPEN_RETURN_PATH" }, >>>>> [MIG_CMD_PING] = { .len = sizeof(uint32_t), >>>>> .name = "PING" }, >>>>> - [MIG_CMD_POSTCOPY_ADVISE] = { .len = 16, .name = >>>>> "POSTCOPY_ADVISE" }, >>>>> + [MIG_CMD_POSTCOPY_ADVISE] = { .len = -1, .name = >>>>> "POSTCOPY_ADVISE" }, >>>>> [MIG_CMD_POSTCOPY_LISTEN] = { .len = 0, .name = >>>>> "POSTCOPY_LISTEN" }, >>>>> [MIG_CMD_POSTCOPY_RUN] = { .len = 0, .name = >>>>> "POSTCOPY_RUN" }, >>>>> [MIG_CMD_POSTCOPY_RAM_DISCARD] = { >>>>> @@ -827,12 +827,17 @@ int qemu_savevm_send_packaged(QEMUFile *f, >>>>> const uint8_t *buf, size_t len) >>>>> /* Send prior to any postcopy transfer */ >>>>> void qemu_savevm_send_postcopy_advise(QEMUFile *f) >>>>> { >>>>> - uint64_t tmp[2]; >>>>> - tmp[0] = cpu_to_be64(getpagesize()); >>>>> - tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); >>>>> + if (migrate_postcopy_ram()) { >>>>> + uint64_t tmp[2]; >>>>> + tmp[0] = cpu_to_be64(getpagesize()); >>>>> + tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); >>>>> - trace_qemu_savevm_send_postcopy_advise(); >>>>> - qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 16, >>>>> (uint8_t *)tmp); >>>>> + trace_qemu_savevm_send_postcopy_advise(); >>>>> + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, >>>>> + 16, (uint8_t *)tmp); >>>>> + } else { >>>>> + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 0, >>>>> NULL); >>>>> + } >>>>> } >>>>> /* Sent prior to starting the destination running in postcopy, >>>>> discard pages >>>> I haven't yet figured out why you are reusing this command with a >>>> different number of parameters. >>>> For this to pass, I need that Dave comment on this. >>>> >>>> So, >>>> >>>> Reviewed-by: Juan Quintela <quintela@redhat.com> >>>> >>>> conditioned that Dave agrees with this. >>> These parameters are unrelated if ram postcopy is disabled. So, I >>> should be >>> better to have a possibility of skipping them, then to send unneeded >>> numbers >>> and write separate code to read them from the stream (rewriting >>> loadvm_postcopy_handle_advise to just read these two numbers and do >>> nothing >>> about ram postcopy for this case). >> I think I'd prefer either a new command or keeping these fields >> (probably all 0 ?) >> my worry is what happens in the case if we add a 3rd postcopy >> subfeature; >> In your case we have three possibilities: >> >> a) Postcopy RAM only - 16 bytes >> b) Postcopy persistent-dirty-bitmap only - 0 bytes >> c) Both - 16 bytes >> >> Lets say we added postcopy-foo in the future and it wanted to add >> another 16 bytes, what would it send if it was >> foo+persistent-dirty-bitmap and no RAM? >> We'd end up with 16 bytes sent but you'd have to be very careful >> never to get that confused with case (a) above. >> >> (I don't feel too strongly about it though) > > We would like to stick current solution if it is not a problem. About > postcopy-foo: on the one hand, the format may be determined from > migration capabilities, on the other hand we can add for example > 4bytes format identifier starting from postcopy-foo and make all > parameters size to be at least 20 bytes. Or something like this. > Ok? > > >> >> Dave >> >>> -- >>> Best regards, >>> Vladimir >>> >> -- >> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > >
* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote: > 01.02.2017 14:06, Vladimir Sementsov-Ogievskiy wrote: > > 24.01.2017 22:53, Dr. David Alan Gilbert wrote: > > > * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote: > > > > 24.01.2017 12:24, Juan Quintela wrote: > > > > > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > > > > > > Split common postcopy staff from ram postcopy staff. > > > > > > > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > > > > > <vsementsov@virtuozzo.com> > > > > > > --- > > > > > > include/migration/migration.h | 1 + > > > > > > migration/migration.c | 39 > > > > > > +++++++++++++++++++++++++++------------ > > > > > > migration/postcopy-ram.c | 4 +++- > > > > > > migration/savevm.c | 31 ++++++++++++++++++++++--------- > > > > > > 4 files changed, 53 insertions(+), 22 deletions(-) > > > > > > > > > > > Hi > > > > > > > > > > { > > > > > > MigrationState *s; > > > > > > @@ -1587,9 +1592,11 @@ static int > > > > > > postcopy_start(MigrationState *ms, bool *old_vm_running) > > > > > > * need to tell the destination to throw any > > > > > > pages it's already received > > > > > > * that are dirty > > > > > > */ > > > > > > - if (ram_postcopy_send_discard_bitmap(ms)) { > > > > > > - error_report("postcopy send discard bitmap failed"); > > > > > > - goto fail; > > > > > > + if (migrate_postcopy_ram()) { > > > > > > + if (ram_postcopy_send_discard_bitmap(ms)) { > > > > > > + error_report("postcopy send discard bitmap failed"); > > > > > > + goto fail; > > > > > > + } > > > > > I will have preffered that for the ram commands, to embed the > > > > > migrate_postocpy_ram() check inside them, but that is taste, and > > > > > everyone has its own O:-) > > > > > > > > > > > diff --git a/migration/savevm.c b/migration/savevm.c > > > > > > index e436cb2..c8a71c8 100644 > > > > > > --- a/migration/savevm.c > > > > > > +++ b/migration/savevm.c > > > > > > @@ -73,7 +73,7 @@ static struct mig_cmd_args { > > > > > > [MIG_CMD_INVALID] = { .len = -1, .name = "INVALID" }, > > > > > > [MIG_CMD_OPEN_RETURN_PATH] = { .len = 0, .name = > > > > > > "OPEN_RETURN_PATH" }, > > > > > > [MIG_CMD_PING] = { .len = > > > > > > sizeof(uint32_t), .name = "PING" }, > > > > > > - [MIG_CMD_POSTCOPY_ADVISE] = { .len = 16, .name = > > > > > > "POSTCOPY_ADVISE" }, > > > > > > + [MIG_CMD_POSTCOPY_ADVISE] = { .len = -1, .name = > > > > > > "POSTCOPY_ADVISE" }, > > > > > > [MIG_CMD_POSTCOPY_LISTEN] = { .len = 0, .name = > > > > > > "POSTCOPY_LISTEN" }, > > > > > > [MIG_CMD_POSTCOPY_RUN] = { .len = 0, .name = > > > > > > "POSTCOPY_RUN" }, > > > > > > [MIG_CMD_POSTCOPY_RAM_DISCARD] = { > > > > > > @@ -827,12 +827,17 @@ int > > > > > > qemu_savevm_send_packaged(QEMUFile *f, const uint8_t > > > > > > *buf, size_t len) > > > > > > /* Send prior to any postcopy transfer */ > > > > > > void qemu_savevm_send_postcopy_advise(QEMUFile *f) > > > > > > { > > > > > > - uint64_t tmp[2]; > > > > > > - tmp[0] = cpu_to_be64(getpagesize()); > > > > > > - tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); > > > > > > + if (migrate_postcopy_ram()) { > > > > > > + uint64_t tmp[2]; > > > > > > + tmp[0] = cpu_to_be64(getpagesize()); > > > > > > + tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); > > > > > > - trace_qemu_savevm_send_postcopy_advise(); > > > > > > - qemu_savevm_command_send(f, > > > > > > MIG_CMD_POSTCOPY_ADVISE, 16, (uint8_t *)tmp); > > > > > > + trace_qemu_savevm_send_postcopy_advise(); > > > > > > + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, > > > > > > + 16, (uint8_t *)tmp); > > > > > > + } else { > > > > > > + qemu_savevm_command_send(f, > > > > > > MIG_CMD_POSTCOPY_ADVISE, 0, NULL); > > > > > > + } > > > > > > } > > > > > > /* Sent prior to starting the destination running in > > > > > > postcopy, discard pages > > > > > I haven't yet figured out why you are reusing this command with a > > > > > different number of parameters. > > > > > For this to pass, I need that Dave comment on this. > > > > > > > > > > So, > > > > > > > > > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > > > > > > > > > conditioned that Dave agrees with this. > > > > These parameters are unrelated if ram postcopy is disabled. So, > > > > I should be > > > > better to have a possibility of skipping them, then to send > > > > unneeded numbers > > > > and write separate code to read them from the stream (rewriting > > > > loadvm_postcopy_handle_advise to just read these two numbers and > > > > do nothing > > > > about ram postcopy for this case). > > > I think I'd prefer either a new command or keeping these fields > > > (probably all 0 ?) > > > my worry is what happens in the case if we add a 3rd postcopy > > > subfeature; > > > In your case we have three possibilities: > > > > > > a) Postcopy RAM only - 16 bytes > > > b) Postcopy persistent-dirty-bitmap only - 0 bytes > > > c) Both - 16 bytes > > > > > > Lets say we added postcopy-foo in the future and it wanted to add > > > another 16 bytes, what would it send if it was > > > foo+persistent-dirty-bitmap and no RAM? > > > We'd end up with 16 bytes sent but you'd have to be very careful > > > never to get that confused with case (a) above. > > > > > > (I don't feel too strongly about it though) > > > > We would like to stick current solution if it is not a problem. About > > postcopy-foo: on the one hand, the format may be determined from > > migration capabilities, on the other hand we can add for example 4bytes > > format identifier starting from postcopy-foo and make all parameters > > size to be at least 20 bytes. Or something like this. > > > > Ok? Yes OK; just please make sure it's all very commented and obvious. Dave > > > > > > > > > > Dave > > > > > > > -- > > > > Best regards, > > > > Vladimir > > > > > > > -- > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > > > > > -- > Best regards, > Vladimir > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/include/migration/migration.h b/include/migration/migration.h index c309d23..d06f6c4 100644 --- a/include/migration/migration.h +++ b/include/migration/migration.h @@ -294,6 +294,7 @@ void migrate_add_blocker(Error *reason); */ void migrate_del_blocker(Error *reason); +bool migrate_postcopy(void); bool migrate_postcopy_ram(void); bool migrate_zero_blocks(void); diff --git a/migration/migration.c b/migration/migration.c index f498ab8..5966dff 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1260,6 +1260,11 @@ bool migrate_postcopy_ram(void) return s->enabled_capabilities[MIGRATION_CAPABILITY_POSTCOPY_RAM]; } +bool migrate_postcopy(void) +{ + return migrate_postcopy_ram(); +} + bool migrate_auto_converge(void) { MigrationState *s; @@ -1587,9 +1592,11 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running) * need to tell the destination to throw any pages it's already received * that are dirty */ - if (ram_postcopy_send_discard_bitmap(ms)) { - error_report("postcopy send discard bitmap failed"); - goto fail; + if (migrate_postcopy_ram()) { + if (ram_postcopy_send_discard_bitmap(ms)) { + error_report("postcopy send discard bitmap failed"); + goto fail; + } } /* @@ -1598,8 +1605,10 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running) * wrap their state up here */ qemu_file_set_rate_limit(ms->to_dst_file, INT64_MAX); - /* Ping just for debugging, helps line traces up */ - qemu_savevm_send_ping(ms->to_dst_file, 2); + if (migrate_postcopy_ram()) { + /* Ping just for debugging, helps line traces up */ + qemu_savevm_send_ping(ms->to_dst_file, 2); + } /* * While loading the device state we may trigger page transfer @@ -1624,7 +1633,9 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running) qemu_savevm_send_postcopy_listen(fb); qemu_savevm_state_complete_precopy(fb, false); - qemu_savevm_send_ping(fb, 3); + if (migrate_postcopy_ram()) { + qemu_savevm_send_ping(fb, 3); + } qemu_savevm_send_postcopy_run(fb); @@ -1647,11 +1658,13 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running) qemu_mutex_unlock_iothread(); - /* - * Although this ping is just for debug, it could potentially be - * used for getting a better measurement of downtime at the source. - */ - qemu_savevm_send_ping(ms->to_dst_file, 4); + if (migrate_postcopy_ram()) { + /* + * Although this ping is just for debug, it could potentially be + * used for getting a better measurement of downtime at the source. + */ + qemu_savevm_send_ping(ms->to_dst_file, 4); + } ret = qemu_file_get_error(ms->to_dst_file); if (ret) { @@ -1802,7 +1815,9 @@ static void *migration_thread(void *opaque) /* And do a ping that will make stuff easier to debug */ qemu_savevm_send_ping(s->to_dst_file, 1); + } + if (migrate_postcopy()) { /* * Tell the destination that we *might* want to do postcopy later; * if the other end can't do postcopy it should fail now, nice and @@ -1836,7 +1851,7 @@ static void *migration_thread(void *opaque) if (pending_size && pending_size >= max_size) { /* Still a significant amount to transfer */ - if (migrate_postcopy_ram() && + if (migrate_postcopy() && s->state != MIGRATION_STATUS_POSTCOPY_ACTIVE && pend_nonpost <= max_size && atomic_read(&s->start_postcopy)) { diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index a40dddb..aef5690 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -339,7 +339,9 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis) } postcopy_state_set(POSTCOPY_INCOMING_END); - migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0); + if (migrate_postcopy_ram()) { + migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0); + } if (mis->postcopy_tmp_page) { munmap(mis->postcopy_tmp_page, getpagesize()); diff --git a/migration/savevm.c b/migration/savevm.c index e436cb2..c8a71c8 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -73,7 +73,7 @@ static struct mig_cmd_args { [MIG_CMD_INVALID] = { .len = -1, .name = "INVALID" }, [MIG_CMD_OPEN_RETURN_PATH] = { .len = 0, .name = "OPEN_RETURN_PATH" }, [MIG_CMD_PING] = { .len = sizeof(uint32_t), .name = "PING" }, - [MIG_CMD_POSTCOPY_ADVISE] = { .len = 16, .name = "POSTCOPY_ADVISE" }, + [MIG_CMD_POSTCOPY_ADVISE] = { .len = -1, .name = "POSTCOPY_ADVISE" }, [MIG_CMD_POSTCOPY_LISTEN] = { .len = 0, .name = "POSTCOPY_LISTEN" }, [MIG_CMD_POSTCOPY_RUN] = { .len = 0, .name = "POSTCOPY_RUN" }, [MIG_CMD_POSTCOPY_RAM_DISCARD] = { @@ -827,12 +827,17 @@ int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len) /* Send prior to any postcopy transfer */ void qemu_savevm_send_postcopy_advise(QEMUFile *f) { - uint64_t tmp[2]; - tmp[0] = cpu_to_be64(getpagesize()); - tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); + if (migrate_postcopy_ram()) { + uint64_t tmp[2]; + tmp[0] = cpu_to_be64(getpagesize()); + tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits()); - trace_qemu_savevm_send_postcopy_advise(); - qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 16, (uint8_t *)tmp); + trace_qemu_savevm_send_postcopy_advise(); + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, + 16, (uint8_t *)tmp); + } else { + qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 0, NULL); + } } /* Sent prior to starting the destination running in postcopy, discard pages @@ -1315,6 +1320,10 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis) return -1; } + if (!migrate_postcopy_ram()) { + return 0; + } + if (!postcopy_ram_supported_by_host()) { return -1; } @@ -1515,7 +1524,9 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis) * A rare case, we entered listen without having to do any discards, * so do the setup that's normally done at the time of the 1st discard. */ - postcopy_ram_prepare_discard(mis); + if (migrate_postcopy_ram()) { + postcopy_ram_prepare_discard(mis); + } } /* @@ -1523,8 +1534,10 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis) * However, at this point the CPU shouldn't be running, and the IO * shouldn't be doing anything yet so don't actually expect requests */ - if (postcopy_ram_enable_notify(mis)) { - return -1; + if (migrate_postcopy_ram()) { + if (postcopy_ram_enable_notify(mis)) { + return -1; + } } if (mis->have_listen_thread) {
Split common postcopy staff from ram postcopy staff. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/migration/migration.h | 1 + migration/migration.c | 39 +++++++++++++++++++++++++++------------ migration/postcopy-ram.c | 4 +++- migration/savevm.c | 31 ++++++++++++++++++++++--------- 4 files changed, 53 insertions(+), 22 deletions(-)