[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] migration: isolate return path on src
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [PATCH] migration: isolate return path on src |
Date: |
Thu, 1 Jun 2017 10:51:13 +0800 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Wed, May 31, 2017 at 08:33:01PM +0200, Juan Quintela wrote:
> "Dr. David Alan Gilbert" <address@hidden> wrote:
> > * Peter Xu (address@hidden) 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 <address@hidden>
> >> ---
> >> 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