Re: [Qemu-devel] [PATCH 03/17] migration: split common postcopy out of ram postcopy
* Vladimir Sementsov-Ogievskiy (vsement...@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 (vsement...@virtuozzo.com) wrote: > > > > 24.01.2017 12:24, Juan Quintela wrote: > > > > > Vladimir Sementsov-Ogievskiywrote: > > > > > > Split common postcopy staff from ram postcopy staff. > > > > > > > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > > > > > > > > > > > --- > > > > > >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 > > > > > > > > > > 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 ?)
Re: [Qemu-devel] [PATCH 03/17] migration: split common postcopy out of ram postcopy
01.02.2017 14:06, Vladimir Sementsov-Ogievskiy wrote: 24.01.2017 22:53, Dr. David Alan Gilbert wrote: * Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote: 24.01.2017 12:24, Juan Quintela wrote: Vladimir Sementsov-Ogievskiywrote: Split common postcopy staff from ram postcopy staff. Signed-off-by: Vladimir Sementsov-Ogievskiy --- 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 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 / dgilb...@redhat.com / Manchester, UK -- Best regards, Vladimir
Re: [Qemu-devel] [PATCH 03/17] migration: split common postcopy out of ram postcopy
24.01.2017 22:53, Dr. David Alan Gilbert wrote: * Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote: 24.01.2017 12:24, Juan Quintela wrote: Vladimir Sementsov-Ogievskiywrote: Split common postcopy staff from ram postcopy staff. Signed-off-by: Vladimir Sementsov-Ogievskiy --- 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 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 / dgilb...@redhat.com / Manchester, UK -- Best regards, Vladimir
Re: [Qemu-devel] [PATCH 03/17] migration: split common postcopy out of ram postcopy
* Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote: > 24.01.2017 22:53, Dr. David Alan Gilbert wrote: > > * Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote: > > > 24.01.2017 12:24, Juan Quintela wrote: > > > > Vladimir Sementsov-Ogievskiywrote: > > > > > Split common postcopy staff from ram postcopy staff. > > > > > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > > > > --- > > > > >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 > > > > > > > > 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
Re: [Qemu-devel] [PATCH 03/17] migration: split common postcopy out of ram postcopy
24.01.2017 22:53, Dr. David Alan Gilbert wrote: * Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote: 24.01.2017 12:24, Juan Quintela wrote: Vladimir Sementsov-Ogievskiywrote: Split common postcopy staff from ram postcopy staff. Signed-off-by: Vladimir Sementsov-Ogievskiy --- 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 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 / dgilb...@redhat.com / Manchester, UK -- Best regards, Vladimir
Re: [Qemu-devel] [PATCH 03/17] migration: split common postcopy out of ram postcopy
* Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote: > 24.01.2017 12:24, Juan Quintela wrote: > > Vladimir Sementsov-Ogievskiywrote: > > > Split common postcopy staff from ram postcopy staff. > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > > --- > > > 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 > > > > 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 / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH 03/17] migration: split common postcopy out of ram postcopy
24.01.2017 12:24, Juan Quintela wrote: Vladimir Sementsov-Ogievskiywrote: Split common postcopy staff from ram postcopy staff. Signed-off-by: Vladimir Sementsov-Ogievskiy --- 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 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). -- Best regards, Vladimir
Re: [Qemu-devel] [PATCH 03/17] migration: split common postcopy out of ram postcopy
Vladimir Sementsov-Ogievskiywrote: > Split common postcopy staff from ram postcopy staff. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > 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 conditioned that Dave agrees with this.