Re: [PATCH v9 02/14] migration: Postcopy preemption preparation on channel creation

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Tue, Jul 19, 2022 at 04:31:20PM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > On Tue, Jul 19, 2022 at 04:15:49PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Peter Xu (peterx@redhat.com) wrote:
> > > > > Create a new socket for postcopy to be prepared to send postcopy 
> > > > > requested
> > > > > pages via this specific channel, so as to not get blocked by precopy 
> > > > > pages.
> > > > > 
> > > > > A new thread is also created on dest qemu to receive data from this 
> > > > > new channel
> > > > > based on the ram_load_postcopy() routine.
> > > > > 
> > > > > The ram_load_postcopy(POSTCOPY) branch and the thread has not started 
> > > > > to
> > > > > function, and that'll be done in follow up patches.
> > > > > 
> > > > > Cleanup the new sockets on both src/dst QEMUs, meanwhile look after 
> > > > > the new
> > > > > thread too to make sure it'll be recycled properly.
> > > > 
> > > > I'm hitting a CI failure here:
> > > > https://gitlab.com/dagrh/qemu/-/jobs/2741659845
> > > > 
> > > > c.o -c ../migration/migration.c
> > > > ../migration/migration.c: In function ‘migration_ioc_process_incoming’:
> > > > ../migration/migration.c:766:8: error: ‘start_migration’ may be used 
> > > > uninitialized in this function [-Werror=maybe-uninitialized]
> > > >   766 |     if (start_migration) {
> > > >       |        ^
> > > > 
> > > > > Reviewed-by: Daniel P. Berrang?? <berrange@redhat.com>
> > > > > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > > ---
> > > > >  migration/migration.c    | 62 +++++++++++++++++++++++----
> > > > >  migration/migration.h    |  8 ++++
> > > > >  migration/postcopy-ram.c | 92 
> > > > > ++++++++++++++++++++++++++++++++++++++--
> > > > >  migration/postcopy-ram.h | 10 +++++
> > > > >  migration/ram.c          | 25 ++++++++---
> > > > >  migration/ram.h          |  4 +-
> > > > >  migration/savevm.c       | 20 ++++-----
> > > > >  migration/socket.c       | 22 +++++++++-
> > > > >  migration/socket.h       |  1 +
> > > > >  migration/trace-events   |  5 ++-
> > > > >  10 files changed, 218 insertions(+), 31 deletions(-)
> > > > > 
> > > > > diff --git a/migration/migration.c b/migration/migration.c
> > > > > index ce7bb68cdc..9484fec0b2 100644
> > > > > --- a/migration/migration.c
> > > > > +++ b/migration/migration.c
> > > > > @@ -321,6 +321,12 @@ void migration_incoming_state_destroy(void)
> > > > >          mis->page_requested = NULL;
> > > > >      }
> > > > >  
> > > > > +    if (mis->postcopy_qemufile_dst) {
> > > > > +        
> > > > > migration_ioc_unregister_yank_from_file(mis->postcopy_qemufile_dst);
> > > > > +        qemu_fclose(mis->postcopy_qemufile_dst);
> > > > > +        mis->postcopy_qemufile_dst = NULL;
> > > > > +    }
> > > > > +
> > > > >      yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> > > > >  }
> > > > >  
> > > > > @@ -714,15 +720,21 @@ void migration_fd_process_incoming(QEMUFile *f, 
> > > > > Error **errp)
> > > > >      migration_incoming_process();
> > > > >  }
> > > > >  
> > > > > +static bool migration_needs_multiple_sockets(void)
> > > > > +{
> > > > > +    return migrate_use_multifd() || migrate_postcopy_preempt();
> > > > > +}
> > > > > +
> > > > >  void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
> > > > >  {
> > > > >      MigrationIncomingState *mis = migration_incoming_get_current();
> > > > >      Error *local_err = NULL;
> > > > >      bool start_migration;
> > > > > +    QEMUFile *f;
> > > > >  
> > > > >      if (!mis->from_src_file) {
> > > > >          /* The first connection (multifd may have multiple) */
> > > > > -        QEMUFile *f = qemu_file_new_input(ioc);
> > > > > +        f = qemu_file_new_input(ioc);
> > > > >  
> > > > >          if (!migration_incoming_setup(f, errp)) {
> > > > >              return;
> > > > > @@ -730,13 +742,18 @@ void migration_ioc_process_incoming(QIOChannel 
> > > > > *ioc, Error **errp)
> > > > >  
> > > > >          /*
> > > > >           * Common migration only needs one channel, so we can start
> > > > > -         * right now.  Multifd needs more than one channel, we wait.
> > > > > +         * right now.  Some features need more than one channel, we 
> > > > > wait.
> > > > >           */
> > > > > -        start_migration = !migrate_use_multifd();
> > > > > +        start_migration = !migration_needs_multiple_sockets();
> > > > >      } else {
> > > > >          /* Multiple connections */
> > > > > -        assert(migrate_use_multifd());
> > > > > -        start_migration = multifd_recv_new_channel(ioc, &local_err);
> > > > > +        assert(migration_needs_multiple_sockets());
> > > > > +        if (migrate_use_multifd()) {
> > > > > +            start_migration = multifd_recv_new_channel(ioc, 
> > > > > &local_err);
> > > > > +        } else if (migrate_postcopy_preempt()) {
> > > > > +            f = qemu_file_new_input(ioc);
> > > > > +            start_migration = postcopy_preempt_new_channel(mis, f);
> > > > > +        }
> > > > 
> > > > So that doesn't always set start_migration?
> > > 
> > > Logically it should, because we asserted on
> > > migration_needs_multiple_sockets(), while multifd and preempt mode are the
> > > only two that may need multiple sockets.  So it must go into either of
> > > them.
> > > 
> > > A fix could be like this:
> > > 
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index 76cf2a72c0..7c7e529ca7 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -753,7 +753,8 @@ void migration_ioc_process_incoming(QIOChannel *ioc, 
> > > Error **errp)
> > >          assert(migration_needs_multiple_sockets());
> > >          if (migrate_use_multifd()) {
> > >              start_migration = multifd_recv_new_channel(ioc, &local_err);
> > > -        } else if (migrate_postcopy_preempt()) {
> > > +        } else {
> > > +            assert(migrate_postcopy_preempt());
> > >              f = qemu_file_new_input(ioc);
> > >              start_migration = postcopy_preempt_new_channel(mis, f);
> > >          }
> > > 
> > > Or we simply set start_migration=false as initial value.
> > > 
> > > Should I repost the series?
> > 
> > No just post a fixup and I'll merge it.
> 
> Attached, thanks.

Thanks,

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

> -- 
> Peter Xu

> From af276d410e140b8d4292dc476163f2734cf089c1 Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@redhat.com>
> Date: Tue, 19 Jul 2022 11:38:32 -0400
> Subject: [PATCH] fixup! migration: Postcopy preemption preparation on channel
>  creation
> Content-type: text/plain
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 76cf2a72c0..7c7e529ca7 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -753,7 +753,8 @@ void migration_ioc_process_incoming(QIOChannel *ioc, 
> Error **errp)
>          assert(migration_needs_multiple_sockets());
>          if (migrate_use_multifd()) {
>              start_migration = multifd_recv_new_channel(ioc, &local_err);
> -        } else if (migrate_postcopy_preempt()) {
> +        } else {
> +            assert(migrate_postcopy_preempt());
>              f = qemu_file_new_input(ioc);
>              start_migration = postcopy_preempt_new_channel(mis, f);
>          }
> -- 
> 2.32.0
> 

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK