[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v9 02/14] migration: Postcopy preemption preparation on chann
From: |
Peter Xu |
Subject: |
Re: [PATCH v9 02/14] migration: Postcopy preemption preparation on channel creation |
Date: |
Tue, 19 Jul 2022 11:39:47 -0400 |
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.
--
Peter Xu
0001-fixup-migration-Postcopy-preemption-preparation-on-c.patch
Description: Text document
[PATCH v9 04/14] migration: Postcopy recover with preempt enabled, Peter Xu, 2022/07/07
[PATCH v9 05/14] migration: Create the postcopy preempt channel asynchronously, Peter Xu, 2022/07/07
[PATCH v9 07/14] migration: Add helpers to detect TLS capability, Peter Xu, 2022/07/07
[PATCH v9 06/14] migration: Add property x-postcopy-preempt-break-huge, Peter Xu, 2022/07/07
[PATCH v9 13/14] tests: Add postcopy tls recovery migration test, Peter Xu, 2022/07/07
[PATCH v9 11/14] tests: Move MigrateCommon upper, Peter Xu, 2022/07/07
[PATCH v9 14/14] tests: Add postcopy preempt tests, Peter Xu, 2022/07/07
[PATCH v9 08/14] migration: Export tls-[creds|hostname|authz] params to cmdline too, Peter Xu, 2022/07/07
[PATCH v9 10/14] migration: Respect postcopy request order in preemption mode, Peter Xu, 2022/07/07