| Message ID | 1496226934-29752-1-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
Peter Xu <peterx@redhat.com> wrote: > There are some places that binded "return path" with postcopy. Let's be > prepared for its usage even without postcopy. This patch mainly did this > on source side. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > standalone patch isolated from the return path series. ok to be picked > up in case one day we'll re-face the return path enablement. > With it, we are ready on source side. The dst side change has been queued. Reviewed-by: Juan Quintela <quintela@redhat.com> queued
* Peter Xu (peterx@redhat.com) wrote: > There are some places that binded "return path" with postcopy. Let's be > prepared for its usage even without postcopy. This patch mainly did this > on source side. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > standalone patch isolated from the return path series. ok to be picked > up in case one day we'll re-face the return path enablement. > With it, we are ready on source side. The dst side change has been queued. > --- > migration/migration.c | 11 ++++++----- > migration/trace-events | 4 ++-- > 2 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index ad29e53..96e549e 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1850,13 +1850,12 @@ static void migration_completion(MigrationState *s, int current_active_state, > * cleaning everything else up (since if there are no failures > * it will wait for the destination to send it's status in > * a SHUT command). > - * Postcopy opens rp if enabled (even if it's not avtivated) > */ > - if (migrate_postcopy_ram()) { > + if (s->rp_state.from_dst_file) { > int rp_error; > - trace_migration_completion_postcopy_end_before_rp(); > + trace_migration_return_path_end_before(); > rp_error = await_return_path_close_on_source(s); > - trace_migration_completion_postcopy_end_after_rp(rp_error); > + trace_migration_return_path_end_after(rp_error); > if (rp_error) { > goto fail_invalidate; > } > @@ -1931,13 +1930,15 @@ static void *migration_thread(void *opaque) > > qemu_savevm_state_header(s->to_dst_file); > > - if (migrate_postcopy_ram()) { > + if (s->to_dst_file) { > /* Now tell the dest that it should open its end so it can reply */ > qemu_savevm_send_open_return_path(s->to_dst_file); > > /* And do a ping that will make stuff easier to debug */ > qemu_savevm_send_ping(s->to_dst_file, 1); I'm confused. The migration_thread on the source will always have a s->to_dst_file there so why test? But also, we can't send the 'open return path' and 'send ping' messages without a guard; sending them to old QEMUs will cause them to fail when they don't know what the message is. I suspect sending them to slightly-old QEMUs will cause them to fail when they try and send a return message if the destination can't send it. I can see a if (s->rp_state.from_dst_file) making some sense if it's done at the right point. Dave > + } > > + if (migrate_postcopy_ram()) { > /* > * 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 > diff --git a/migration/trace-events b/migration/trace-events > index 5b8ccf3..38345be 100644 > --- a/migration/trace-events > +++ b/migration/trace-events > @@ -88,8 +88,8 @@ migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d" > migration_completion_file_err(void) "" > migration_completion_postcopy_end(void) "" > migration_completion_postcopy_end_after_complete(void) "" > -migration_completion_postcopy_end_before_rp(void) "" > -migration_completion_postcopy_end_after_rp(int rp_error) "%d" > +migration_return_path_end_before(void) "" > +migration_return_path_end_after(int rp_error) "%d" > migration_thread_after_loop(void) "" > migration_thread_file_err(void) "" > migration_thread_setup_complete(void) "" > -- > 2.7.4 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Peter Xu (peterx@redhat.com) wrote: >> There are some places that binded "return path" with postcopy. Let's be >> prepared for its usage even without postcopy. This patch mainly did this >> on source side. >> >> Signed-off-by: Peter Xu <peterx@redhat.com> >> --- >> standalone patch isolated from the return path series. ok to be picked >> up in case one day we'll re-face the return path enablement. >> With it, we are ready on source side. The dst side change has been queued. >> --- >> migration/migration.c | 11 ++++++----- >> migration/trace-events | 4 ++-- >> 2 files changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index ad29e53..96e549e 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -1850,13 +1850,12 @@ static void migration_completion(MigrationState *s, int current_active_state, >> * cleaning everything else up (since if there are no failures >> * it will wait for the destination to send it's status in >> * a SHUT command). >> - * Postcopy opens rp if enabled (even if it's not avtivated) >> */ >> - if (migrate_postcopy_ram()) { >> + if (s->rp_state.from_dst_file) { >> int rp_error; >> - trace_migration_completion_postcopy_end_before_rp(); >> + trace_migration_return_path_end_before(); >> rp_error = await_return_path_close_on_source(s); >> - trace_migration_completion_postcopy_end_after_rp(rp_error); >> + trace_migration_return_path_end_after(rp_error); >> if (rp_error) { >> goto fail_invalidate; >> } >> @@ -1931,13 +1930,15 @@ static void *migration_thread(void *opaque) >> >> qemu_savevm_state_header(s->to_dst_file); >> >> - if (migrate_postcopy_ram()) { >> + if (s->to_dst_file) { >> /* Now tell the dest that it should open its end so it can reply */ >> qemu_savevm_send_open_return_path(s->to_dst_file); >> >> /* And do a ping that will make stuff easier to debug */ >> qemu_savevm_send_ping(s->to_dst_file, 1); > > I'm confused. > The migration_thread on the source will always have a s->to_dst_file > there so why test? > > But also, we can't send the 'open return path' and 'send ping' messages > without a guard; sending them to old QEMUs will cause them to fail when > they don't know what the message is. I suspect sending them to > slightly-old QEMUs will cause them to fail when they try and send a > return message if the destination can't send it. > > I can see a if (s->rp_state.from_dst_file) making some sense if it's > done at the right point. You are right. Second chunk makes no sense. I told Peter to split the "objectification" of migration state and this patch. This made more sense when the user *had* required a return path. Good catch. Thanks, Juan.
On Wed, May 31, 2017 at 08:33:01PM +0200, Juan Quintela wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > * Peter Xu (peterx@redhat.com) wrote: > >> There are some places that binded "return path" with postcopy. Let's be > >> prepared for its usage even without postcopy. This patch mainly did this > >> on source side. > >> > >> Signed-off-by: Peter Xu <peterx@redhat.com> > >> --- > >> standalone patch isolated from the return path series. ok to be picked > >> up in case one day we'll re-face the return path enablement. > >> With it, we are ready on source side. The dst side change has been queued. > >> --- > >> migration/migration.c | 11 ++++++----- > >> migration/trace-events | 4 ++-- > >> 2 files changed, 8 insertions(+), 7 deletions(-) > >> > >> diff --git a/migration/migration.c b/migration/migration.c > >> index ad29e53..96e549e 100644 > >> --- a/migration/migration.c > >> +++ b/migration/migration.c > >> @@ -1850,13 +1850,12 @@ static void migration_completion(MigrationState *s, int current_active_state, > >> * cleaning everything else up (since if there are no failures > >> * it will wait for the destination to send it's status in > >> * a SHUT command). > >> - * Postcopy opens rp if enabled (even if it's not avtivated) > >> */ > >> - if (migrate_postcopy_ram()) { > >> + if (s->rp_state.from_dst_file) { > >> int rp_error; > >> - trace_migration_completion_postcopy_end_before_rp(); > >> + trace_migration_return_path_end_before(); > >> rp_error = await_return_path_close_on_source(s); > >> - trace_migration_completion_postcopy_end_after_rp(rp_error); > >> + trace_migration_return_path_end_after(rp_error); > >> if (rp_error) { > >> goto fail_invalidate; > >> } > >> @@ -1931,13 +1930,15 @@ static void *migration_thread(void *opaque) > >> > >> qemu_savevm_state_header(s->to_dst_file); > >> > >> - if (migrate_postcopy_ram()) { > >> + if (s->to_dst_file) { > >> /* Now tell the dest that it should open its end so it can reply */ > >> qemu_savevm_send_open_return_path(s->to_dst_file); > >> > >> /* And do a ping that will make stuff easier to debug */ > >> qemu_savevm_send_ping(s->to_dst_file, 1); > > > > I'm confused. > > The migration_thread on the source will always have a s->to_dst_file > > there so why test? > > > > But also, we can't send the 'open return path' and 'send ping' messages > > without a guard; sending them to old QEMUs will cause them to fail when > > they don't know what the message is. I suspect sending them to > > slightly-old QEMUs will cause them to fail when they try and send a > > return message if the destination can't send it. > > > > I can see a if (s->rp_state.from_dst_file) making some sense if it's > > done at the right point. > > You are right. > > Second chunk makes no sense. I told Peter to split the > "objectification" of migration state and this patch. This made more > sense when the user *had* required a return path. Oops! I believe it should be what Dave mentioned (s->rp_state.from_dst_file rather than s->to_dst_file). Sorry! I'll repost to see whether we'd like the new one. Thanks,
diff --git a/migration/migration.c b/migration/migration.c index ad29e53..96e549e 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1850,13 +1850,12 @@ static void migration_completion(MigrationState *s, int current_active_state, * cleaning everything else up (since if there are no failures * it will wait for the destination to send it's status in * a SHUT command). - * Postcopy opens rp if enabled (even if it's not avtivated) */ - if (migrate_postcopy_ram()) { + if (s->rp_state.from_dst_file) { int rp_error; - trace_migration_completion_postcopy_end_before_rp(); + trace_migration_return_path_end_before(); rp_error = await_return_path_close_on_source(s); - trace_migration_completion_postcopy_end_after_rp(rp_error); + trace_migration_return_path_end_after(rp_error); if (rp_error) { goto fail_invalidate; } @@ -1931,13 +1930,15 @@ static void *migration_thread(void *opaque) qemu_savevm_state_header(s->to_dst_file); - if (migrate_postcopy_ram()) { + if (s->to_dst_file) { /* Now tell the dest that it should open its end so it can reply */ qemu_savevm_send_open_return_path(s->to_dst_file); /* And do a ping that will make stuff easier to debug */ qemu_savevm_send_ping(s->to_dst_file, 1); + } + if (migrate_postcopy_ram()) { /* * 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 diff --git a/migration/trace-events b/migration/trace-events index 5b8ccf3..38345be 100644 --- a/migration/trace-events +++ b/migration/trace-events @@ -88,8 +88,8 @@ migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d" migration_completion_file_err(void) "" migration_completion_postcopy_end(void) "" migration_completion_postcopy_end_after_complete(void) "" -migration_completion_postcopy_end_before_rp(void) "" -migration_completion_postcopy_end_after_rp(int rp_error) "%d" +migration_return_path_end_before(void) "" +migration_return_path_end_after(int rp_error) "%d" migration_thread_after_loop(void) "" migration_thread_file_err(void) "" migration_thread_setup_complete(void) ""
There are some places that binded "return path" with postcopy. Let's be prepared for its usage even without postcopy. This patch mainly did this on source side. Signed-off-by: Peter Xu <peterx@redhat.com> --- standalone patch isolated from the return path series. ok to be picked up in case one day we'll re-face the return path enablement. With it, we are ready on source side. The dst side change has been queued. --- migration/migration.c | 11 ++++++----- migration/trace-events | 4 ++-- 2 files changed, 8 insertions(+), 7 deletions(-)