migration/migration.c | 11 ++++++----- migration/trace-events | 4 ++-- 2 files changed, 8 insertions(+), 7 deletions(-)
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);
+ }
+ 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
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,
--
Peter Xu
© 2016 - 2024 Red Hat, Inc.