[v4] migration: Postcopy Preemption

[PATCH v4 01/19] migration: Postpone releasing MigrationState.hostname

Posted by Peter Xu 3 years, 10 months ago

We used to release it right after migrate_fd_connect().  That's not good
enough when there're more than one socket pair required, because it'll be
needed to establish TLS connection for the rest channels.

One example is multifd, where we copied over the hostname for each channel
but that's actually not needed.

Keeping the hostname until the cleanup phase of migration.

Cc: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/channel.c   | 1 -
 migration/migration.c | 5 +++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/migration/channel.c b/migration/channel.c
index c4fc000a1a..c6a8dcf1d7 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -96,6 +96,5 @@ void migration_channel_connect(MigrationState *s,
         }
     }
     migrate_fd_connect(s, error);
-    g_free(s->hostname);
     error_free(error);
 }
diff --git a/migration/migration.c b/migration/migration.c
index 695f0f2900..281d33326b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1809,6 +1809,11 @@ static void migrate_fd_cleanup(MigrationState *s)
     qemu_bh_delete(s->cleanup_bh);
     s->cleanup_bh = NULL;
 
+    if (s->hostname) {
+        g_free(s->hostname);
+        s->hostname = NULL;
+    }
+
     qemu_savevm_state_cleanup();
 
     if (s->to_dst_file) {
-- 
2.32.0

Re: [PATCH v4 01/19] migration: Postpone releasing MigrationState.hostname

Posted by Daniel P. Berrangé 3 years, 9 months ago

On Thu, Mar 31, 2022 at 11:08:39AM -0400, Peter Xu wrote:
> We used to release it right after migrate_fd_connect().  That's not good
> enough when there're more than one socket pair required, because it'll be
> needed to establish TLS connection for the rest channels.
> 
> One example is multifd, where we copied over the hostname for each channel
> but that's actually not needed.
> 
> Keeping the hostname until the cleanup phase of migration.
> 
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/channel.c   | 1 -
>  migration/migration.c | 5 +++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/channel.c b/migration/channel.c
> index c4fc000a1a..c6a8dcf1d7 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -96,6 +96,5 @@ void migration_channel_connect(MigrationState *s,
>          }
>      }
>      migrate_fd_connect(s, error);
> -    g_free(s->hostname);
>      error_free(error);
>  }
> diff --git a/migration/migration.c b/migration/migration.c
> index 695f0f2900..281d33326b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1809,6 +1809,11 @@ static void migrate_fd_cleanup(MigrationState *s)
>      qemu_bh_delete(s->cleanup_bh);
>      s->cleanup_bh = NULL;
>  
> +    if (s->hostname) {
> +        g_free(s->hostname);
> +        s->hostname = NULL;
> +    }

FWIW there's a marginally more concise pattern:

  g_clear_pointer(&s->hostname, g_free)


Either way

   Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 01/19] migration: Postpone releasing MigrationState.hostname

Posted by Peter Xu 3 years, 9 months ago

On Wed, Apr 20, 2022 at 11:34:16AM +0100, Daniel P. Berrangé wrote:
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 695f0f2900..281d33326b 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1809,6 +1809,11 @@ static void migrate_fd_cleanup(MigrationState *s)
> >      qemu_bh_delete(s->cleanup_bh);
> >      s->cleanup_bh = NULL;
> >  
> > +    if (s->hostname) {
> > +        g_free(s->hostname);
> > +        s->hostname = NULL;
> > +    }
> 
> FWIW there's a marginally more concise pattern:
> 
>   g_clear_pointer(&s->hostname, g_free)

Sounds good.

> 
> 
> Either way
> 
>    Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Thanks,

-- 
Peter Xu

Re: [PATCH v4 01/19] migration: Postpone releasing MigrationState.hostname

Posted by Dr. David Alan Gilbert 3 years, 10 months ago

* Peter Xu (peterx@redhat.com) wrote:
> We used to release it right after migrate_fd_connect().  That's not good
> enough when there're more than one socket pair required, because it'll be
> needed to establish TLS connection for the rest channels.
> 
> One example is multifd, where we copied over the hostname for each channel
> but that's actually not needed.
> 
> Keeping the hostname until the cleanup phase of migration.
> 
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/channel.c   | 1 -
>  migration/migration.c | 5 +++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/channel.c b/migration/channel.c
> index c4fc000a1a..c6a8dcf1d7 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -96,6 +96,5 @@ void migration_channel_connect(MigrationState *s,
>          }
>      }
>      migrate_fd_connect(s, error);
> -    g_free(s->hostname);
>      error_free(error);
>  }
> diff --git a/migration/migration.c b/migration/migration.c
> index 695f0f2900..281d33326b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1809,6 +1809,11 @@ static void migrate_fd_cleanup(MigrationState *s)
>      qemu_bh_delete(s->cleanup_bh);
>      s->cleanup_bh = NULL;
>  
> +    if (s->hostname) {
> +        g_free(s->hostname);
> +        s->hostname = NULL;
> +    }
> +
>      qemu_savevm_state_cleanup();
>  
>      if (s->to_dst_file) {
> -- 
> 2.32.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK