| Message ID | 20250215123119.814345-3-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Allow to enable multifd and postcopy migration together | expand |
Prasad Pandit <ppandit@redhat.com> writes: > From: Prasad Pandit <pjp@fedoraproject.org> > > Enable Multifd and Postcopy migration together. > The migration_ioc_process_incoming() routine > checks magic value sent on each channel and > helps to properly setup multifd and postcopy > channels. > > The Precopy and Multifd threads work during the > initial guest RAM transfer. When migration moves > to the Postcopy phase, the multifd threads are > shutdown and Postcopy threads on the destination > request/pull data from the source side. > > Signed-off-by: Prasad Pandit <pjp@fedoraproject.org> > --- > migration/migration.c | 107 ++++++++++++++++++++----------------- > migration/multifd-nocomp.c | 3 +- > migration/multifd.c | 4 +- > migration/options.c | 5 -- > migration/ram.c | 7 ++- > 5 files changed, 64 insertions(+), 62 deletions(-) > > v6: > - Shutdown multifd threads before postcopy_start() > - Reorder tests/qtest/migration/ patches > - Some refactoring of functions > > v5: > - https://lore.kernel.org/qemu-devel/20250205122712.229151-1-ppandit@redhat.com/T/#t > > diff --git a/migration/migration.c b/migration/migration.c > index 396928513a..38697182e8 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -95,6 +95,9 @@ enum mig_rp_message_type { > MIG_RP_MSG_MAX > }; > > +/* Migration channel types */ > +enum { CH_MAIN, CH_MULTIFD, CH_POSTCOPY }; > + > /* When we add fault tolerance, we could have several > migrations at once. For now we don't need to add > dynamic creation of migration */ > @@ -959,28 +962,19 @@ void migration_fd_process_incoming(QEMUFile *f) > migration_incoming_process(); > } > > -/* > - * Returns true when we want to start a new incoming migration process, > - * false otherwise. > - */ > -static bool migration_should_start_incoming(bool main_channel) > +static bool migration_has_main_and_multifd_channels(void) > { > - /* Multifd doesn't start unless all channels are established */ > - if (migrate_multifd()) { > - return migration_has_all_channels(); > + MigrationIncomingState *mis = migration_incoming_get_current(); > + if (!mis->from_src_file) { > + /* main channel not established */ > + return false; > } > > - /* Preempt channel only starts when the main channel is created */ > - if (migrate_postcopy_preempt()) { > - return main_channel; > + if (migrate_multifd() && !multifd_recv_all_channels_created()) { > + return false; > } > > - /* > - * For all the rest types of migration, we should only reach here when > - * it's the main channel that's being created, and we should always > - * proceed with this channel. > - */ > - assert(main_channel); > + /* main channel and all multifd channels are established */ > return true; > } How will this avoid peeking the preempt channel? You're assuming preempt is mutually exclusive with multifd it seems. Otherwise you could get the preempt channel creation racing with multifd channels creation. > > @@ -989,13 +983,12 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp) > MigrationIncomingState *mis = migration_incoming_get_current(); > Error *local_err = NULL; > QEMUFile *f; > - bool default_channel = true; > uint32_t channel_magic = 0; > + uint8_t channel = CH_MAIN; > int ret = 0; > > - if (migrate_multifd() && !migrate_mapped_ram() && > - !migrate_postcopy_ram() && > - qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) { > + if (!migration_has_main_and_multifd_channels()) { I'm not entirely sure we need these checks here. They will happen anyway later. Could this be replaced by migration_needs_multiple_sockets() instead? And I'd put this whole channel discovery business in channel.c since it's encoding several assumptions about channels. Some helpers used here might need to be exported, but that's ok. Also, please make a separate patch, we need to be really confident that changing the discovery code around won't introduce any regression, and if it does, we'll want it separate from the postcopy+multifd enablement. It's ok if you have the patch assume that multifd+postcopy will happen later in the series. > + if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) { > /* > * With multiple channels, it is possible that we receive channels > * out of order on destination side, causing incorrect mapping of > @@ -1006,42 +999,58 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp) > * tls handshake while initializing main channel so with tls this > * issue is not possible. > */ This comment block needs to be indented properly. > - ret = migration_channel_read_peek(ioc, (void *)&channel_magic, > - sizeof(channel_magic), errp); > + ret = migration_channel_read_peek(ioc, (void *)&channel_magic, > + sizeof(channel_magic), errp); > + if (ret != 0) { > + return; > + } > > - if (ret != 0) { > - return; > + if (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)) { > + channel = CH_MAIN; > + } else if (channel_magic == cpu_to_be32(MULTIFD_MAGIC)) { > + channel = CH_MULTIFD; > + } else if (!mis->from_src_file > + && mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) { The usual style is to keep the && on the previous line. > + /* reconnect default channel for postcopy recovery */ s/default/main/ > + channel = CH_MAIN; > + } else { > + error_report("%s: unknown channel magic: %u", > + __func__, channel_magic); > + return; This needs to set errp instead of reporting. > + } > + } else if (mis->from_src_file > + && (!strcmp(ioc->name, "migration-tls-incoming") > + || !strcmp(ioc->name, "migration-file-incoming"))) { > + channel = CH_MULTIFD; This is quite misleading. These channels are used without multifd as well. For instance, file-backed fd migration goes past this because !mis->from_src_file but it still uses the file channel. I agree with Peter that checking for channel names is not ideal. I don't see an alternative at the moment (hiding the assumption is of course not a fix). Maybe check migrate_multifd() here and explain in a comment that at the moment, the non-peekable channels happen to be used with multifd only. > } No else clause here and in the rest of the patch makes this is as opaque as the previous version, IMO. We need to define what's supposed to happen whenever the conditionals don't match. Is it an error, g_assert_not_reached(), a fallthrough? Better to set CH_MAIN explicitly wherenever that's the case. > - > - default_channel = (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)); > - } else { > - default_channel = !mis->from_src_file; > + } else if (mis->from_src_file) { > + channel = CH_POSTCOPY; > } Same here. > > if (multifd_recv_setup(errp) != 0) { > return; > } > > - if (default_channel) { > + if (channel == CH_MAIN) { > f = qemu_file_new_input(ioc); > migration_incoming_setup(f); > - } else { > + } else if (channel == CH_MULTIFD) { > /* Multiple connections */ > - assert(migration_needs_multiple_sockets()); > if (migrate_multifd()) { > multifd_recv_new_channel(ioc, &local_err); > - } else { > - assert(migrate_postcopy_preempt()); > - f = qemu_file_new_input(ioc); > - postcopy_preempt_new_channel(mis, f); > } > if (local_err) { > error_propagate(errp, local_err); > return; > } > + } else if (channel == CH_POSTCOPY) { > + assert(migrate_postcopy_preempt()); > + assert(!mis->postcopy_qemufile_dst); > + f = qemu_file_new_input(ioc); > + postcopy_preempt_new_channel(mis, f); > } else { g_assert_not_reached(); } > > - if (migration_should_start_incoming(default_channel)) { > + if (channel != CH_POSTCOPY && migration_has_main_and_multifd_channels()) { You could check CH_POSTCOPY first in the block above this one and return early. > /* If it's a recovery, we're done */ > if (postcopy_try_recover()) { > return; > @@ -1058,20 +1067,15 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp) > */ > bool migration_has_all_channels(void) > { > + if (!migration_has_main_and_multifd_channels()) { > + return false; > + } > + > MigrationIncomingState *mis = migration_incoming_get_current(); > - > - if (!mis->from_src_file) { > + if (migrate_postcopy_preempt() && !mis->postcopy_qemufile_dst) { > return false; > } > > - if (migrate_multifd()) { > - return multifd_recv_all_channels_created(); > - } > - > - if (migrate_postcopy_preempt()) { > - return mis->postcopy_qemufile_dst != NULL; > - } > - > return true; > } > > @@ -1482,6 +1486,8 @@ static void migrate_fd_cleanup(MigrationState *s) > > assert(!migration_is_active()); > > + file_cleanup_outgoing_migration(); > + socket_cleanup_outgoing_migration(); > if (s->state == MIGRATION_STATUS_CANCELLING) { > migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING, > MIGRATION_STATUS_CANCELLED); > @@ -3373,8 +3379,11 @@ static MigIterateState migration_iteration_run(MigrationState *s) > } > > /* Still a significant amount to transfer */ > - if (!in_postcopy && must_precopy <= s->threshold_size && can_switchover && > - qatomic_read(&s->start_postcopy)) { > + if (!in_postcopy && must_precopy <= s->threshold_size > + && can_switchover && qatomic_read(&s->start_postcopy)) { > + if (migrate_multifd()) { > + multifd_send_shutdown(); > + } > if (postcopy_start(s, &local_err)) { > migrate_set_error(s, local_err); > error_report_err(local_err); > diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c > index 1325dba97c..d0edec7cd1 100644 > --- a/migration/multifd-nocomp.c > +++ b/migration/multifd-nocomp.c > @@ -16,6 +16,7 @@ > #include "file.h" > #include "multifd.h" > #include "options.h" > +#include "migration.h" > #include "qapi/error.h" > #include "qemu/cutils.h" > #include "qemu/error-report.h" > @@ -391,7 +392,7 @@ int multifd_ram_flush_and_sync(QEMUFile *f) > MultiFDSyncReq req; > int ret; > > - if (!migrate_multifd()) { > + if (!migrate_multifd() || migration_in_postcopy()) { > return 0; > } > > diff --git a/migration/multifd.c b/migration/multifd.c > index 97ce831775..fa83a43778 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -467,8 +467,6 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) > > static void multifd_send_cleanup_state(void) > { > - file_cleanup_outgoing_migration(); > - socket_cleanup_outgoing_migration(); > qemu_sem_destroy(&multifd_send_state->channels_created); > qemu_sem_destroy(&multifd_send_state->channels_ready); > g_free(multifd_send_state->params); > @@ -481,7 +479,7 @@ void multifd_send_shutdown(void) > { > int i; > > - if (!migrate_multifd()) { > + if (!migrate_multifd() || !multifd_send_state) { > return; > } > > diff --git a/migration/options.c b/migration/options.c > index 4db340b502..c4dfe89edd 100644 > --- a/migration/options.c > +++ b/migration/options.c > @@ -480,11 +480,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp) > error_setg(errp, "Postcopy is not compatible with ignore-shared"); > return false; > } > - > - if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) { > - error_setg(errp, "Postcopy is not yet compatible with multifd"); > - return false; > - } > } > > if (new_caps[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) { > diff --git a/migration/ram.c b/migration/ram.c > index 6f460fd22d..8f22745aba 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -1297,7 +1297,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss) > pss->page = 0; > pss->block = QLIST_NEXT_RCU(pss->block, next); > if (!pss->block) { > - if (multifd_ram_sync_per_round()) { > + if (multifd_ram_sync_per_round() && !migration_in_postcopy()) { > QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel; > int ret = multifd_ram_flush_and_sync(f); > if (ret < 0) { > @@ -1971,9 +1971,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss) > } > } > > - if (migrate_multifd()) { > - RAMBlock *block = pss->block; > - return ram_save_multifd_page(block, offset); > + if (migrate_multifd() && !migration_in_postcopy()) { > + return ram_save_multifd_page(pss->block, offset); > } > > if (control_save_page(pss, offset, &res)) { > -- > 2.48.1
Hello Fabiano, On Tue, 18 Feb 2025 at 03:20, Fabiano Rosas <farosas@suse.de> wrote: > > -static bool migration_should_start_incoming(bool main_channel) > > +static bool migration_has_main_and_multifd_channels(void) > > { ... > > + /* main channel and all multifd channels are established */ > > return true; > > } > > How will this avoid peeking the preempt channel? You're assuming preempt > is mutually exclusive with multifd it seems. Otherwise you could get the > preempt channel creation racing with multifd channels creation. * IIUC postcopy preempt channel is created towards the end of the migration when the Postcopy phase starts. At that time, the 'main' and 'multifd' channels are already established and working. Having the 'main' and when multifd is enabled 'multifd' channels in place is a requirement for starting new migration. So when both the 'main' and 'multifd' channels are established, the new incoming connection is seen as the 'postcopy' one; And it falls to the 'else' block of the 'if' conditional -> if (!migration_has_main_and_multifd_channels()) { * When does postcopy preempt channel creation race with the multifd channel creation? > > @@ -989,13 +983,12 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp) > > + if (!migration_has_main_and_multifd_channels()) { > I'm not entirely sure we need these checks here. They will happen anyway > later. Could this be replaced by migration_needs_multiple_sockets() > instead? * migration_needs_multiple_sockets() => return migrate_multifd() || migrate_postcopy_preempt(); * That logical OR should have been AND, no? It returns true even when one of them is true. That's not multiple types (multifd/postcopy) of sockets. I don't think it helps much. * Let's try this: - First differentiation: peekable Vs non-peekable channels - Peekable channels - Main - Multifd - Non-peekable channels - Postcopy preempt - TLS - File/mapped_ram if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) { if (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)) { channel = CH_MAIN; } else if (channel_magic == cpu_to_be32(MULTIFD_MAGIC)) { channel = CH_MULTIFD; } else { if (!strcmp(ioc->name, "migration-tls-incoming") || !strcmp(ioc->name, "migration-file-incoming"))) { channel = CH_MULTIFD; } else { channel = CH_POSTCOPY; } } * With above, the 'main' channel shall have to send 'magic value' even for reconnection during the postcopy recovery phase. If all channels were consistent and sent a magic value, this code would be much simpler and we may not have to care/worry about the _order_ in which these connections are made. if (channel == 'main') process_main_channel() else if (channel == 'multifd') process_multifd_channel() else if (channel == 'tls') process_tls_channel() else if (channel == 'file') process_file_channel() else if (channel == 'postcopy') process_postcopy_channel() > And I'd put this whole channel discovery business in channel.c since > it's encoding several assumptions about channels. Some helpers used here > might need to be exported, but that's ok. > > Also, please make a separate patch, we need to be really confident that > changing the discovery code around won't introduce any regression, and > if it does, we'll want it separate from the postcopy+multifd > enablement. It's ok if you have the patch assume that multifd+postcopy > will happen later in the series. * TBH, I think we have complicated this whole thing with multiple channel types, their inconsistent behaviour and dependence on the _order_ in which connections are made. Do we really need channel types? Could we consider rationalising them? > > + if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) { > > /* > > * With multiple channels, it is possible that we receive channels > > * out of order on destination side, causing incorrect mapping of > > @@ -1006,42 +999,58 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp) > > * tls handshake while initializing main channel so with tls this > > * issue is not possible. > > */ > > This comment block needs to be indented properly. > > > - ret = migration_channel_read_peek(ioc, (void *)&channel_magic, > > - sizeof(channel_magic), errp); > > + ret = migration_channel_read_peek(ioc, (void *)&channel_magic, > > + sizeof(channel_magic), errp); > > + if (ret != 0) { > > + return; > > + } > > > > - if (ret != 0) { > > - return; > > + if (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)) { > > + channel = CH_MAIN; > > + } else if (channel_magic == cpu_to_be32(MULTIFD_MAGIC)) { > > + channel = CH_MULTIFD; > > + } else if (!mis->from_src_file > > + && mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) { > > The usual style is to keep the && on the previous line. > > > + /* reconnect default channel for postcopy recovery */ > > s/default/main/ * Okay, will fix these. > > + channel = CH_MAIN; > > + } else { > > + error_report("%s: unknown channel magic: %u", > > + __func__, channel_magic); > > + return; > > This needs to set errp instead of reporting. * Okay. > > + } > > + } else if (mis->from_src_file > > + && (!strcmp(ioc->name, "migration-tls-incoming") > > + || !strcmp(ioc->name, "migration-file-incoming"))) { > > + channel = CH_MULTIFD; > > This is quite misleading. These channels are used without multifd as > well. For instance, file-backed fd migration goes past this because > !mis->from_src_file but it still uses the file channel. > > I agree with Peter that checking for channel names is not ideal. I don't > see an alternative at the moment (hiding the assumption is of course not > a fix). Maybe check migrate_multifd() here and explain in a comment that > at the moment, the non-peekable channels happen to be used with multifd > only. * The first paragraph says these channels are used without migrate_multifd(); And the second paragraph says they are used with migrate_multifd() only....?? === } else { /* Multiple connections */ assert(migration_needs_multiple_sockets()); if (migrate_multifd()) { multifd_recv_new_channel(ioc, &local_err); } else { assert(migrate_postcopy_preempt()); f = qemu_file_new_input(ioc); postcopy_preempt_new_channel(mis, f); } === * IIUC in the current code, when migrate_multifd() is true, these channels get processed via - multifd_recv_new_channel(). And when migrate_multifd() is false, it falls to postcopy_preempt_new_channel() part. Not sure if/how that works really. It is possible that currently these (tls/file) channels are used only with migrate_multifd() enabled and so are processed with multifd_recv_new_channel() function. The current patch handles them the same way. * About checking channel names, in the non-peekable category above, how do we differentiate between 'TLS', 'File' and 'Postcopy' channels? Without magic values, we don't have much choice really. And seeing those names in the code also tells the reader that 'TLS' and 'File' channels are processed as CH_MULTIFD via - multifd_recv_new_channel(). > No else clause here and in the rest of the patch makes this is as opaque > as the previous version, IMO. We need to define what's supposed to > happen whenever the conditionals don't match. Is it an error, > g_assert_not_reached(), a fallthrough? Better to set CH_MAIN explicitly > wherenever that's the case. * I'd say let's treat unmatched conditions as an error and return. > > - default_channel = (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)); > > - } else { > > - default_channel = !mis->from_src_file; > > + } else if (mis->from_src_file) { > > + channel = CH_POSTCOPY; > > } > > Same here. * Here final else means the main channel (mis->from_src_file) is not established, so it defaults to CH_MAIN. > > You could check CH_POSTCOPY first in the block above this one and return > early. > * Okay. * My request is the same - let's think about having consistent channel behaviour. The restructuring and overhauling of the channel processing part could be done separately, outside the current scope of enabling multifd+postcopy together with this series. Let's not try to fix everything in one go, in one series. Thank you. --- - Prasad
Hi Prasad, On 2025-02-15 18:01, Prasad Pandit wrote: > From: Prasad Pandit <pjp@fedoraproject.org> > > Enable Multifd and Postcopy migration together. > The migration_ioc_process_incoming() routine > checks magic value sent on each channel and > helps to properly setup multifd and postcopy > channels. > > The Precopy and Multifd threads work during the > initial guest RAM transfer. When migration moves > to the Postcopy phase, the multifd threads are > shutdown and Postcopy threads on the destination > request/pull data from the source side. > > Signed-off-by: Prasad Pandit <pjp@fedoraproject.org> > --- > migration/migration.c | 107 ++++++++++++++++++++----------------- > migration/multifd-nocomp.c | 3 +- > migration/multifd.c | 4 +- > migration/options.c | 5 -- > migration/ram.c | 7 ++- > 5 files changed, 64 insertions(+), 62 deletions(-) > > v6: > - Shutdown multifd threads before postcopy_start() > - Reorder tests/qtest/migration/ patches > - Some refactoring of functions > > v5: > - https://lore.kernel.org/qemu-devel/20250205122712.229151-1-ppandit@redhat.com/T/#t > > diff --git a/migration/migration.c b/migration/migration.c > index 396928513a..38697182e8 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -95,6 +95,9 @@ enum mig_rp_message_type { > MIG_RP_MSG_MAX > }; > > +/* Migration channel types */ > +enum { CH_MAIN, CH_MULTIFD, CH_POSTCOPY }; > + > /* When we add fault tolerance, we could have several > migrations at once. For now we don't need to add > dynamic creation of migration */ > @@ -959,28 +962,19 @@ void migration_fd_process_incoming(QEMUFile *f) > migration_incoming_process(); > } > > -/* > - * Returns true when we want to start a new incoming migration process, > - * false otherwise. > - */ > -static bool migration_should_start_incoming(bool main_channel) > +static bool migration_has_main_and_multifd_channels(void) > { > - /* Multifd doesn't start unless all channels are established */ > - if (migrate_multifd()) { > - return migration_has_all_channels(); > + MigrationIncomingState *mis = migration_incoming_get_current(); > + if (!mis->from_src_file) { > + /* main channel not established */ > + return false; > } > > - /* Preempt channel only starts when the main channel is created */ > - if (migrate_postcopy_preempt()) { > - return main_channel; > + if (migrate_multifd() && !multifd_recv_all_channels_created()) { > + return false; > } > > - /* > - * For all the rest types of migration, we should only reach here when > - * it's the main channel that's being created, and we should always > - * proceed with this channel. > - */ > - assert(main_channel); > + /* main channel and all multifd channels are established */ > return true; > } > > @@ -989,13 +983,12 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp) > MigrationIncomingState *mis = migration_incoming_get_current(); > Error *local_err = NULL; > QEMUFile *f; > - bool default_channel = true; > uint32_t channel_magic = 0; > + uint8_t channel = CH_MAIN; > int ret = 0; > > - if (migrate_multifd() && !migrate_mapped_ram() && > - !migrate_postcopy_ram() && > - qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) { > + if (!migration_has_main_and_multifd_channels()) { > + if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) { > /* > * With multiple channels, it is possible that we receive channels > * out of order on destination side, causing incorrect mapping of > @@ -1006,42 +999,58 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp) > * tls handshake while initializing main channel so with tls this > * issue is not possible. > */ > - ret = migration_channel_read_peek(ioc, (void *)&channel_magic, > - sizeof(channel_magic), errp); > + ret = migration_channel_read_peek(ioc, (void *)&channel_magic, > + sizeof(channel_magic), errp); > + if (ret != 0) { > + return; > + } > > - if (ret != 0) { > - return; > + if (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)) { > + channel = CH_MAIN; > + } else if (channel_magic == cpu_to_be32(MULTIFD_MAGIC)) { > + channel = CH_MULTIFD; > + } else if (!mis->from_src_file > + && mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) { > + /* reconnect default channel for postcopy recovery */ > + channel = CH_MAIN; > + } else { > + error_report("%s: unknown channel magic: %u", > + __func__, channel_magic); Here, the number reported in the error will have incorrect endianness on a non-BE system. I think it would be better to convert channel_magic to the system endianness right after reading it. On top of that, then there is no need to convert constants with magic numbers when comparing. > + return; > + } > + } else if (mis->from_src_file > + && (!strcmp(ioc->name, "migration-tls-incoming") > + || !strcmp(ioc->name, "migration-file-incoming"))) { > + channel = CH_MULTIFD; > } > - > - default_channel = (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)); > - } else { > - default_channel = !mis->from_src_file; > + } else if (mis->from_src_file) { > + channel = CH_POSTCOPY; > } > > if (multifd_recv_setup(errp) != 0) { > return; > } > > - if (default_channel) { > + if (channel == CH_MAIN) { > f = qemu_file_new_input(ioc); > migration_incoming_setup(f); > - } else { > + } else if (channel == CH_MULTIFD) { > /* Multiple connections */ > - assert(migration_needs_multiple_sockets()); > if (migrate_multifd()) { > multifd_recv_new_channel(ioc, &local_err); > - } else { > - assert(migrate_postcopy_preempt()); > - f = qemu_file_new_input(ioc); > - postcopy_preempt_new_channel(mis, f); > } > if (local_err) { > error_propagate(errp, local_err); > return; > } > + } else if (channel == CH_POSTCOPY) { > + assert(migrate_postcopy_preempt()); > + assert(!mis->postcopy_qemufile_dst); > + f = qemu_file_new_input(ioc); > + postcopy_preempt_new_channel(mis, f); > } > > - if (migration_should_start_incoming(default_channel)) { > + if (channel != CH_POSTCOPY && migration_has_main_and_multifd_channels()) { > /* If it's a recovery, we're done */ > if (postcopy_try_recover()) { > return; > @@ -1058,20 +1067,15 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp) > */ > bool migration_has_all_channels(void) > { > + if (!migration_has_main_and_multifd_channels()) { > + return false; > + } > + > MigrationIncomingState *mis = migration_incoming_get_current(); > - > - if (!mis->from_src_file) { > + if (migrate_postcopy_preempt() && !mis->postcopy_qemufile_dst) { > return false; > } > > - if (migrate_multifd()) { > - return multifd_recv_all_channels_created(); > - } > - > - if (migrate_postcopy_preempt()) { > - return mis->postcopy_qemufile_dst != NULL; > - } > - > return true; > } > > @@ -1482,6 +1486,8 @@ static void migrate_fd_cleanup(MigrationState *s) > > assert(!migration_is_active()); > > + file_cleanup_outgoing_migration(); > + socket_cleanup_outgoing_migration(); > if (s->state == MIGRATION_STATUS_CANCELLING) { > migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING, > MIGRATION_STATUS_CANCELLED); > @@ -3373,8 +3379,11 @@ static MigIterateState migration_iteration_run(MigrationState *s) > } > > /* Still a significant amount to transfer */ > - if (!in_postcopy && must_precopy <= s->threshold_size && can_switchover && > - qatomic_read(&s->start_postcopy)) { > + if (!in_postcopy && must_precopy <= s->threshold_size > + && can_switchover && qatomic_read(&s->start_postcopy)) { > + if (migrate_multifd()) { > + multifd_send_shutdown(); > + } > if (postcopy_start(s, &local_err)) { > migrate_set_error(s, local_err); > error_report_err(local_err); > diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c > index 1325dba97c..d0edec7cd1 100644 > --- a/migration/multifd-nocomp.c > +++ b/migration/multifd-nocomp.c > @@ -16,6 +16,7 @@ > #include "file.h" > #include "multifd.h" > #include "options.h" > +#include "migration.h" > #include "qapi/error.h" > #include "qemu/cutils.h" > #include "qemu/error-report.h" > @@ -391,7 +392,7 @@ int multifd_ram_flush_and_sync(QEMUFile *f) > MultiFDSyncReq req; > int ret; > > - if (!migrate_multifd()) { > + if (!migrate_multifd() || migration_in_postcopy()) { > return 0; > } > > diff --git a/migration/multifd.c b/migration/multifd.c > index 97ce831775..fa83a43778 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -467,8 +467,6 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) > > static void multifd_send_cleanup_state(void) > { > - file_cleanup_outgoing_migration(); > - socket_cleanup_outgoing_migration(); > qemu_sem_destroy(&multifd_send_state->channels_created); > qemu_sem_destroy(&multifd_send_state->channels_ready); > g_free(multifd_send_state->params); > @@ -481,7 +479,7 @@ void multifd_send_shutdown(void) > { > int i; > > - if (!migrate_multifd()) { > + if (!migrate_multifd() || !multifd_send_state) { > return; > } > > diff --git a/migration/options.c b/migration/options.c > index 4db340b502..c4dfe89edd 100644 > --- a/migration/options.c > +++ b/migration/options.c > @@ -480,11 +480,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp) > error_setg(errp, "Postcopy is not compatible with ignore-shared"); > return false; > } > - > - if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) { > - error_setg(errp, "Postcopy is not yet compatible with multifd"); > - return false; > - } > } > > if (new_caps[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) { > diff --git a/migration/ram.c b/migration/ram.c > index 6f460fd22d..8f22745aba 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -1297,7 +1297,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss) > pss->page = 0; > pss->block = QLIST_NEXT_RCU(pss->block, next); > if (!pss->block) { > - if (multifd_ram_sync_per_round()) { > + if (multifd_ram_sync_per_round() && !migration_in_postcopy()) { > QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel; > int ret = multifd_ram_flush_and_sync(f); > if (ret < 0) { > @@ -1971,9 +1971,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss) > } > } > > - if (migrate_multifd()) { > - RAMBlock *block = pss->block; > - return ram_save_multifd_page(block, offset); > + if (migrate_multifd() && !migration_in_postcopy()) { > + return ram_save_multifd_page(pss->block, offset); > } > > if (control_save_page(pss, offset, &res)) { > -- > 2.48.1 > >
Prasad Pandit <ppandit@redhat.com> writes: > Hello Fabiano, > > On Tue, 18 Feb 2025 at 03:20, Fabiano Rosas <farosas@suse.de> wrote: >> > -static bool migration_should_start_incoming(bool main_channel) >> > +static bool migration_has_main_and_multifd_channels(void) >> > { > ... >> > + /* main channel and all multifd channels are established */ >> > return true; >> > } >> >> How will this avoid peeking the preempt channel? You're assuming preempt >> is mutually exclusive with multifd it seems. Otherwise you could get the >> preempt channel creation racing with multifd channels creation. > > * IIUC postcopy preempt channel is created towards the end of the > migration when the Postcopy phase starts. At that time, the 'main' and > 'multifd' channels are already established and working. Having the > 'main' and when multifd is enabled 'multifd' channels in place is a > requirement for starting new migration. So when both the 'main' and > 'multifd' channels are established, the new incoming connection is > seen as the 'postcopy' one; And it falls to the 'else' block of the > 'if' conditional -> if (!migration_has_main_and_multifd_channels()) { > Do you concede that this code has a hidden assumption? Either that migrate_multifd() != migrate_postcopy_preempt() or that multifd channels must be set up before postcopy preempt channel? Because that is enough for us to have to do something about it. Either restructuring or a comment explaining. > * When does postcopy preempt channel creation race with the multifd > channel creation? > Well, taking that this is the destination side, I don't think we can predict when the other end of the migration will open a channel, can we? For instance, postcopy_do_resume() has this comment: /* * If preempt is enabled, re-establish the preempt channel. Note that * we do it after resume prepare to make sure the main channel will be * created before the preempt channel. E.g. with weak network, the * dest QEMU may get messed up with the preempt and main channels on * the order of connection setup. This guarantees the correct order. */ It looks like if the main channel can race, so do the multifd channels, no? In any case, I'm fine with just documenting any assumption for now. >> > @@ -989,13 +983,12 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp) >> > + if (!migration_has_main_and_multifd_channels()) { >> I'm not entirely sure we need these checks here. They will happen anyway >> later. Could this be replaced by migration_needs_multiple_sockets() >> instead? > > * migration_needs_multiple_sockets() => return migrate_multifd() || > migrate_postcopy_preempt(); > > * That logical OR should have been AND, no? It returns true even when > one of them is true. That's not multiple types (multifd/postcopy) of > sockets. I don't think it helps much. > Nope, this is just saying whether a single channel is expected, or more than one. That's why I think it would be a good gate for this peeking code. Since postcopy preempt could be a peekable channel, it's misleading to put it all behind QIO_CHANNEL_FEATURE_READ_MSG_PEEK only. This is a time-bomb for the next person to refactor this code. > * Let's try this: > - First differentiation: peekable Vs non-peekable channels As I said above, this is not entirely what we're doing. > - Peekable channels > - Main > - Multifd > - Non-peekable channels > - Postcopy preempt > - TLS > - File/mapped_ram > > if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) > { > if (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)) { > channel = CH_MAIN; > } else if (channel_magic == cpu_to_be32(MULTIFD_MAGIC)) { > channel = CH_MULTIFD; > } else { > if (!strcmp(ioc->name, "migration-tls-incoming") > || !strcmp(ioc->name, "migration-file-incoming"))) { > channel = CH_MULTIFD; > } else { > channel = CH_POSTCOPY; > } > } > > * With above, the 'main' channel shall have to send 'magic value' even > for reconnection during the postcopy recovery phase. If all channels > were consistent and sent a magic value, this code would be much > simpler and we may not have to care/worry about the _order_ in which > these connections are made. > Right, but that's not what we have today. Changing this requires figuring out how to keep the stream compatible when channels now start sending extra stuff at the start. It's not trivial. There's also mapped-ram which is asynchronous and there might be something special to be done about the TLS handshake, I'm not sure. > if (channel == 'main') > process_main_channel() > else if (channel == 'multifd') > process_multifd_channel() > else if (channel == 'tls') > process_tls_channel() > else if (channel == 'file') > process_file_channel() > else if (channel == 'postcopy') > process_postcopy_channel() > >> And I'd put this whole channel discovery business in channel.c since >> it's encoding several assumptions about channels. Some helpers used here >> might need to be exported, but that's ok. >> >> Also, please make a separate patch, we need to be really confident that >> changing the discovery code around won't introduce any regression, and >> if it does, we'll want it separate from the postcopy+multifd >> enablement. It's ok if you have the patch assume that multifd+postcopy >> will happen later in the series. > > * TBH, I think we have complicated this whole thing with multiple > channel types, their inconsistent behaviour and dependence on the > _order_ in which connections are made. Do we really need channel > types? Could we consider rationalising them? > Well, aside from preempt, they're *not* dependent on the order. That's the point of having to do all of this dance. In fact we might be better off if we could serialize the connections somehow. I havent't followed this series closely, could you point me to the discussion that led to the channels concept being introduced? >> > + if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) { >> > /* >> > * With multiple channels, it is possible that we receive channels >> > * out of order on destination side, causing incorrect mapping of >> > @@ -1006,42 +999,58 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp) >> > * tls handshake while initializing main channel so with tls this >> > * issue is not possible. >> > */ >> >> This comment block needs to be indented properly. >> >> > - ret = migration_channel_read_peek(ioc, (void *)&channel_magic, >> > - sizeof(channel_magic), errp); >> > + ret = migration_channel_read_peek(ioc, (void *)&channel_magic, >> > + sizeof(channel_magic), errp); >> > + if (ret != 0) { >> > + return; >> > + } >> > >> > - if (ret != 0) { >> > - return; >> > + if (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)) { >> > + channel = CH_MAIN; >> > + } else if (channel_magic == cpu_to_be32(MULTIFD_MAGIC)) { >> > + channel = CH_MULTIFD; >> > + } else if (!mis->from_src_file >> > + && mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) { >> >> The usual style is to keep the && on the previous line. >> >> > + /* reconnect default channel for postcopy recovery */ >> >> s/default/main/ > > * Okay, will fix these. > > >> > + channel = CH_MAIN; >> > + } else { >> > + error_report("%s: unknown channel magic: %u", >> > + __func__, channel_magic); >> > + return; >> >> This needs to set errp instead of reporting. > > * Okay. > >> > + } >> > + } else if (mis->from_src_file >> > + && (!strcmp(ioc->name, "migration-tls-incoming") >> > + || !strcmp(ioc->name, "migration-file-incoming"))) { >> > + channel = CH_MULTIFD; >> >> This is quite misleading. These channels are used without multifd as >> well. For instance, file-backed fd migration goes past this because >> !mis->from_src_file but it still uses the file channel. >> >> I agree with Peter that checking for channel names is not ideal. I don't >> see an alternative at the moment (hiding the assumption is of course not >> a fix). Maybe check migrate_multifd() here and explain in a comment that >> at the moment, the non-peekable channels happen to be used with multifd >> only. > > * The first paragraph says these channels are used without > migrate_multifd(); And the second paragraph says they are used with > migrate_multifd() only....?? Yes. They *can* be used without multifd. The comment would explain that at that point in the code, these are the only types possible. So as to not mislead future readers that whenever tls/file, then multifd must be used. > === > } else { > /* Multiple connections */ > assert(migration_needs_multiple_sockets()); > if (migrate_multifd()) { > multifd_recv_new_channel(ioc, &local_err); > } else { > assert(migrate_postcopy_preempt()); > f = qemu_file_new_input(ioc); > postcopy_preempt_new_channel(mis, f); > } See? Multifd mutually exclusive with postcopy preempt. You carried that assumption (well done), but made it more subtle (not good), since if/else is by definition showing the relationship between the two while migration_has_main_and_multifd_channels() makes it hidden under the multifd check allowing the last return true to happen. If we're enabling multifd along with postcopy, we need to be aware that the relationship with preempt might not hold true anymore. > === > * IIUC in the current code, when migrate_multifd() is true, these > channels get processed via - multifd_recv_new_channel(). And when > migrate_multifd() is false, it falls to postcopy_preempt_new_channel() > part. This relies on the fact that the management layer will certainly have set multifd on both sides of the migration and that multifd will not be used with postcopy. >Not sure if/how that works really. It is possible that currently > these (tls/file) channels are used only with migrate_multifd() enabled > and so are processed with multifd_recv_new_channel() function. The > current patch handles them the same way. > That's the entire point I'm making when I ask to not omit the else clauses. The tls and file channels can be the main channel as well, but that is never explicit in the code. That uint8 channel = CH_MAIN up there is doing some heavy-lifting. > * About checking channel names, in the non-peekable category above, > how do we differentiate between 'TLS', 'File' and 'Postcopy' channels? > Without magic values, we don't have much choice really. Indeed. > And seeing > those names in the code also tells the reader that 'TLS' and 'File' > channels are processed as CH_MULTIFD via - multifd_recv_new_channel(). We shouldn't rely on the underlying iochannel type. That's why they expose the QIO_CHANNEL_FEATUREs. It's fine if we double check at that point that the channels are either file/tls, but that cannot be the algorithm. The current code can work without relying on it after all and I don't think your postcopy enablement affects that particular part. > >> No else clause here and in the rest of the patch makes this is as opaque >> as the previous version, IMO. We need to define what's supposed to >> happen whenever the conditionals don't match. Is it an error, >> g_assert_not_reached(), a fallthrough? Better to set CH_MAIN explicitly >> wherenever that's the case. > > * I'd say let's treat unmatched conditions as an error and return. > >> > - default_channel = (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)); >> > - } else { >> > - default_channel = !mis->from_src_file; >> > + } else if (mis->from_src_file) { >> > + channel = CH_POSTCOPY; >> > } >> >> Same here. > > * Here final else means the main channel (mis->from_src_file) is not > established, so it defaults to CH_MAIN. > Yes, but let's make it explicit. We have this thing in our minds now. In 6 months time no one remembers anything anymore. >> >> You could check CH_POSTCOPY first in the block above this one and return >> early. >> > > * Okay. > > * My request is the same - let's think about having consistent channel > behaviour. The restructuring and overhauling of the channel processing > part could be done separately, outside the current scope of enabling > multifd+postcopy together with this series. Let's not try to fix > everything in one go, in one series. > Ok, no one is asking for this to be done in one go. You're free to split any side efforts into a new series and proceed with that. Having to do prerequisite work is pretty common. Do you think this series can work without touching the channel discovery code? As I said earlier, I'm missing a bit of context, but to me it seems it cannot. If instead of this refactoring you want to start working on a model for consistent channel advertisement, then that's fine. But we'll have to put this series on hold (which is also fine). It also looks like it could be considerably more work, although I haven't looked at it in detail. Granted, it's work that makes sense, instead of the heuristics we have today. > > Thank you. > --- > - Prasad
On Tue, 18 Feb 2025 at 16:47, Juraj Marcin <jmarcin@redhat.com> wrote: > > + error_report("%s: unknown channel magic: %u", > > + __func__, channel_magic); > > Here, the number reported in the error will have incorrect endianness on > a non-BE system. I think it would be better to convert channel_magic to > the system endianness right after reading it. On top of that, then there > is no need to convert constants with magic numbers when comparing. * Okay. Thank you. --- - Prasad
Hello Fabiano, On Tue, 18 Feb 2025 at 19:52, Fabiano Rosas <farosas@suse.de> wrote: > Do you concede that this code has a hidden assumption? Either that > migrate_multifd() != migrate_postcopy_preempt() or that multifd channels > must be set up before postcopy preempt channel? Because that is enough > for us to have to do something about it. Either restructuring or a > comment explaining. * Not a hidden assumption, but it is an observation that 'main' and 'multifd' channels are established before 'postcopy' ones. And for new migration to start, it is necessary that 'main' and 'multifd' channels (when enabled) are established before migration starts. > > * When does postcopy preempt channel creation race with the multifd > > channel creation? > > For instance, postcopy_do_resume() has this comment: > /* > * If preempt is enabled, re-establish the preempt channel. Note that > * we do it after resume prepare to make sure the main channel will be > * created before the preempt channel. E.g. with weak network, the > * dest QEMU may get messed up with the preempt and main channels on > * the order of connection setup. This guarantees the correct order. > */ > It looks like if the main channel can race, so do the multifd channels, > no? In any case, I'm fine with just documenting any assumption for now. * The first requirement for this race to occur is that two types of channels are created together at the same time. Let's see: * Postcopy migration: without multifd enabled - 'main' channel is created before the migration starts. And 'postcopy' channels are created towards the end of precopy migration, when the Postcopy phase starts. So in this scenario the race does not happen. * Postcopy resume: without multifd enabled - As described in the comment above, preempt channel is created _after_ the 'main' channel to avoid the race condition. * Postcopy migration: with multifd enabled - 'main' and 'multifd' channels are created before migration starts. And 'postcopy' channels are created towards the end of precopy migration, when the Postcopy phase starts. No race occurs. * Postcopy resume: with multifd enabled - 'multifd' channels are shutdown before Postcopy starts, ie. no 'multifd' channels exist during Postcopy resume. So no race between 'postcopy' and 'multifd' channels. - And 'postcopy' channels are created after the 'main' channel to avoid the race between them. - postcopy_do_resume() does not seem to create 'multifd' channels. * Multifd migration: without Postcopy enabled - 'main' and 'multifd' channels are created before the migration starts. They both send 'magic value' bytes, so are easier to differentiate. No race occurs. > > * migration_needs_multiple_sockets() => return migrate_multifd() || > > migrate_postcopy_preempt(); > > > Nope, this is just saying whether a single channel is expected, or more > than one. * If we read it as a question: - migration_needs_multiple_sockets() ? True => Yes, migration needs multiple sockets. - migration_needs_multiple_sockets() ? False => No, migration does not need multiple sockets. Then it should return 'True' when both migrate_multifd() and postcopy_preempt() are enabled. >That's why I think it would be a good gate for this peeking > code. Since postcopy preempt could be a peekable channel, it's > misleading to put it all behind QIO_CHANNEL_FEATURE_READ_MSG_PEEK > only. This is a time-bomb for the next person to refactor this code. * Postcopy preempt could be a peekable channel ? Currently it does not send magic value, does it? > Right, but that's not what we have today. Changing this requires > figuring out how to keep the stream compatible when channels now start > sending extra stuff at the start. It's not trivial. There's also > mapped-ram which is asynchronous and there might be something special to > be done about the TLS handshake, I'm not sure. * True, it's not trivial. > Well, aside from preempt, they're *not* dependent on the order. That's > the point of having to do all of this dance. In fact we might be better > off if we could serialize the connections somehow. > > I havent't followed this series closely, could you point me to the > discussion that led to the channels concept being introduced? * Channels concept was not introduced in this series. It has been there since the beginning, no? > Yes. They *can* be used without multifd. The comment would explain that > at that point in the code, these are the only types possible. So as to > not mislead future readers that whenever tls/file, then multifd must be > used. .... > See? Multifd mutually exclusive with postcopy preempt. You carried that > assumption (well done), but made it more subtle (not good), since > if/else is by definition showing the relationship between the two while > migration_has_main_and_multifd_channels() makes it hidden under the > multifd check allowing the last return true to happen. > > If we're enabling multifd along with postcopy, we need to be aware that > the relationship with preempt might not hold true anymore. * Sorry, I did not get that. Enabling them together means that they are _not_ exclusive, no? It is not Either 'multifd' OR 'postcopy' case, anymore. >>Not sure if/how that works really. It is possible that currently >> these (tls/file) channels are used only with migrate_multifd() enabled >> and so are processed with multifd_recv_new_channel() function. The >> current patch handles them the same way. > > That's the entire point I'm making when I ask to not omit the else > clauses. * ie. we set 'channel = CH_MAIN' in the final else clause as well? - Okay. > Do you think this series can work without touching the channel discovery > code? As I said earlier, I'm missing a bit of context, but to me it > seems it cannot. * The reason we need to touch the channel discovery part is: with 'multifd' and 'postcopy' both enabled, towards the end of migration, when 'postcopy' connection comes in migration_ioc_process_incoming(...) { if (migrate_multifd() && !migrate_mapped_ram() && !migrate_postcopy_ram() && qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) { ... } else { default_channel = !mis->from_src_file; } ... if (migrate_multifd()) { multifd_recv_new_channel(ioc, &local_err); } else { postcopy_preempt_new_channel(mis, f); } } * The first 'if (migrate_multifd() ... !migrate_postcopy_ram()') evaluates to false, in the else part 'default_channel' also evaluates to false, because the 'main' channel is established. Now the new incoming connection falls in the second migrate_multifd() block and gets processed via - multifd_recv_new_channel(ioc, &local_err); call and migration would not complete/finish. * To identify the incoming postcopy connection, in the very first version of this series, a magic value for the postcopy channel was introduced and everything else remained the same. * Would that be an acceptable solution for now? > If instead of this refactoring you want to start working on a model for > consistent channel advertisement, then that's fine. But we'll have to > put this series on hold (which is also fine). It also looks like it > could be considerably more work, although I haven't looked at it in > detail. Granted, it's work that makes sense, instead of the heuristics > we have today. * IMHO, we need not put this series on hold, for now we could go ahead with the postcopy magic value patch if that works. And the larger overhaul of the channel discovery part could be done as a separate series of its own. Thank you. --- - Prasad
Prasad Pandit <ppandit@redhat.com> writes: > Hello Fabiano, > > On Tue, 18 Feb 2025 at 19:52, Fabiano Rosas <farosas@suse.de> wrote: >> Do you concede that this code has a hidden assumption? Either that >> migrate_multifd() != migrate_postcopy_preempt() or that multifd channels >> must be set up before postcopy preempt channel? Because that is enough >> for us to have to do something about it. Either restructuring or a >> comment explaining. > > * Not a hidden assumption, but it is an observation that 'main' and > 'multifd' channels are established before 'postcopy' ones. And for new > migration to start, it is necessary that 'main' and 'multifd' channels > (when enabled) are established before migration starts. > >> > * When does postcopy preempt channel creation race with the multifd >> > channel creation? >> >> For instance, postcopy_do_resume() has this comment: >> /* >> * If preempt is enabled, re-establish the preempt channel. Note that >> * we do it after resume prepare to make sure the main channel will be >> * created before the preempt channel. E.g. with weak network, the >> * dest QEMU may get messed up with the preempt and main channels on >> * the order of connection setup. This guarantees the correct order. >> */ >> It looks like if the main channel can race, so do the multifd channels, >> no? In any case, I'm fine with just documenting any assumption for now. > > * The first requirement for this race to occur is that two types of > channels are created together at the same time. Let's see: > > * Postcopy migration: without multifd enabled > - 'main' channel is created before the migration starts. And > 'postcopy' channels are created towards the end of precopy migration, > when the Postcopy phase starts. So in this scenario the race does not > happen. > > * Postcopy resume: without multifd enabled > - As described in the comment above, preempt channel is created > _after_ the 'main' channel to avoid the race condition. > > * Postcopy migration: with multifd enabled > - 'main' and 'multifd' channels are created before migration > starts. And 'postcopy' channels are created towards the end of precopy > migration, when the Postcopy phase starts. No race occurs. > > * Postcopy resume: with multifd enabled > - 'multifd' channels are shutdown before Postcopy starts, ie. no > 'multifd' channels exist during Postcopy resume. So no race between > 'postcopy' and 'multifd' channels. > - And 'postcopy' channels are created after the 'main' channel > to avoid the race between them. > - postcopy_do_resume() does not seem to create 'multifd' channels. > > * Multifd migration: without Postcopy enabled > - 'main' and 'multifd' channels are created before the migration > starts. They both send 'magic value' bytes, so are easier to > differentiate. No race occurs. > I don't see anything stopping postcopy_start() from being called in the source in relation to multifd recv threads being setup in the destination. So far it seems possible that the source is opening the preempt channel while multifd still hasn't seen all threads. There's also pre-7.2 machines which create the postcopy channel early. > >> > * migration_needs_multiple_sockets() => return migrate_multifd() || >> > migrate_postcopy_preempt(); >> > >> Nope, this is just saying whether a single channel is expected, or more >> than one. > > * If we read it as a question: > - migration_needs_multiple_sockets() ? True => Yes, migration > needs multiple sockets. > - migration_needs_multiple_sockets() ? False => No, migration does > not need multiple sockets. > > Then it should return 'True' when both migrate_multifd() and > postcopy_preempt() are enabled. > Why? >>That's why I think it would be a good gate for this peeking >> code. Since postcopy preempt could be a peekable channel, it's >> misleading to put it all behind QIO_CHANNEL_FEATURE_READ_MSG_PEEK >> only. This is a time-bomb for the next person to refactor this code. > > * Postcopy preempt could be a peekable channel ? Currently it does not > send magic value, does it? > Peekable means it can read ahead and rollback without consuming that part of the stream. We need it because there's code later on that will validate the MAGIC. >> Right, but that's not what we have today. Changing this requires >> figuring out how to keep the stream compatible when channels now start >> sending extra stuff at the start. It's not trivial. There's also >> mapped-ram which is asynchronous and there might be something special to >> be done about the TLS handshake, I'm not sure. > > * True, it's not trivial. > >> Well, aside from preempt, they're *not* dependent on the order. That's >> the point of having to do all of this dance. In fact we might be better >> off if we could serialize the connections somehow. >> >> I havent't followed this series closely, could you point me to the >> discussion that led to the channels concept being introduced? > > * Channels concept was not introduced in this series. It has been > there since the beginning, no? > I thought you meant the CH_MAIN stuff. So now I don't know what you mean. You want to do away with multifd? >> Yes. They *can* be used without multifd. The comment would explain that >> at that point in the code, these are the only types possible. So as to >> not mislead future readers that whenever tls/file, then multifd must be >> used. > .... >> See? Multifd mutually exclusive with postcopy preempt. You carried that >> assumption (well done), but made it more subtle (not good), since >> if/else is by definition showing the relationship between the two while >> migration_has_main_and_multifd_channels() makes it hidden under the >> multifd check allowing the last return true to happen. >> >> If we're enabling multifd along with postcopy, we need to be aware that >> the relationship with preempt might not hold true anymore. > > * Sorry, I did not get that. Enabling them together means that they > are _not_ exclusive, no? It is not Either 'multifd' OR 'postcopy' > case, anymore. > Yes, as we're seeing, there's this assumption that multifd is never enabled along with postcopy_preempt. Now with multifd+postcopy we need to be careful to stop the places where that assumption was made. >>>Not sure if/how that works really. It is possible that currently >>> these (tls/file) channels are used only with migrate_multifd() enabled >>> and so are processed with multifd_recv_new_channel() function. The >>> current patch handles them the same way. >> >> That's the entire point I'm making when I ask to not omit the else >> clauses. > > * ie. we set 'channel = CH_MAIN' in the final else clause as well? - Okay. > >> Do you think this series can work without touching the channel discovery >> code? As I said earlier, I'm missing a bit of context, but to me it >> seems it cannot. > > * The reason we need to touch the channel discovery part is: with > 'multifd' and 'postcopy' both enabled, towards the end of migration, > when 'postcopy' connection comes in > migration_ioc_process_incoming(...) > { > if (migrate_multifd() && !migrate_mapped_ram() && > !migrate_postcopy_ram() && > qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) { > ... > } else { > default_channel = !mis->from_src_file; > } > ... > if (migrate_multifd()) { > multifd_recv_new_channel(ioc, &local_err); > } else { > postcopy_preempt_new_channel(mis, f); > } > } > > * The first 'if (migrate_multifd() ... !migrate_postcopy_ram()') > evaluates to false, in the else part 'default_channel' also evaluates > to false, because the 'main' channel is established. Now the new > incoming connection falls in the second migrate_multifd() block and > gets processed via - multifd_recv_new_channel(ioc, &local_err); call > and migration would not complete/finish. > > * To identify the incoming postcopy connection, in the very first > version of this series, a magic value for the postcopy channel was > introduced and everything else remained the same. > > * Would that be an acceptable solution for now? > Continue with this patch and fix the stuff I mentioned. You can ignore the first two paragraphs of that reply. https://lore.kernel.org/r/87y0y4tf5q.fsf@suse.de I still think we need to test that preempt + multifd scenario, but it should be easy to write a test for that once the series is in more of a final shape. >> If instead of this refactoring you want to start working on a model for >> consistent channel advertisement, then that's fine. But we'll have to >> put this series on hold (which is also fine). It also looks like it >> could be considerably more work, although I haven't looked at it in >> detail. Granted, it's work that makes sense, instead of the heuristics >> we have today. > > * IMHO, we need not put this series on hold, for now we could go ahead > with the postcopy magic value patch if that works. And the larger > overhaul of the channel discovery part could be done as a separate > series of its own. > We can't add magic values, as we've discussed. > Thank you. > --- > - Prasad
diff --git a/migration/migration.c b/migration/migration.c index 396928513a..38697182e8 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -95,6 +95,9 @@ enum mig_rp_message_type { MIG_RP_MSG_MAX }; +/* Migration channel types */ +enum { CH_MAIN, CH_MULTIFD, CH_POSTCOPY }; + /* When we add fault tolerance, we could have several migrations at once. For now we don't need to add dynamic creation of migration */ @@ -959,28 +962,19 @@ void migration_fd_process_incoming(QEMUFile *f) migration_incoming_process(); } -/* - * Returns true when we want to start a new incoming migration process, - * false otherwise. - */ -static bool migration_should_start_incoming(bool main_channel) +static bool migration_has_main_and_multifd_channels(void) { - /* Multifd doesn't start unless all channels are established */ - if (migrate_multifd()) { - return migration_has_all_channels(); + MigrationIncomingState *mis = migration_incoming_get_current(); + if (!mis->from_src_file) { + /* main channel not established */ + return false; } - /* Preempt channel only starts when the main channel is created */ - if (migrate_postcopy_preempt()) { - return main_channel; + if (migrate_multifd() && !multifd_recv_all_channels_created()) { + return false; } - /* - * For all the rest types of migration, we should only reach here when - * it's the main channel that's being created, and we should always - * proceed with this channel. - */ - assert(main_channel); + /* main channel and all multifd channels are established */ return true; } @@ -989,13 +983,12 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp) MigrationIncomingState *mis = migration_incoming_get_current(); Error *local_err = NULL; QEMUFile *f; - bool default_channel = true; uint32_t channel_magic = 0; + uint8_t channel = CH_MAIN; int ret = 0; - if (migrate_multifd() && !migrate_mapped_ram() && - !migrate_postcopy_ram() && - qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) { + if (!migration_has_main_and_multifd_channels()) { + if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) { /* * With multiple channels, it is possible that we receive channels * out of order on destination side, causing incorrect mapping of @@ -1006,42 +999,58 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp) * tls handshake while initializing main channel so with tls this * issue is not possible. */ - ret = migration_channel_read_peek(ioc, (void *)&channel_magic, - sizeof(channel_magic), errp); + ret = migration_channel_read_peek(ioc, (void *)&channel_magic, + sizeof(channel_magic), errp); + if (ret != 0) { + return; + } - if (ret != 0) { - return; + if (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)) { + channel = CH_MAIN; + } else if (channel_magic == cpu_to_be32(MULTIFD_MAGIC)) { + channel = CH_MULTIFD; + } else if (!mis->from_src_file + && mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) { + /* reconnect default channel for postcopy recovery */ + channel = CH_MAIN; + } else { + error_report("%s: unknown channel magic: %u", + __func__, channel_magic); + return; + } + } else if (mis->from_src_file + && (!strcmp(ioc->name, "migration-tls-incoming") + || !strcmp(ioc->name, "migration-file-incoming"))) { + channel = CH_MULTIFD; } - - default_channel = (channel_magic == cpu_to_be32(QEMU_VM_FILE_MAGIC)); - } else { - default_channel = !mis->from_src_file; + } else if (mis->from_src_file) { + channel = CH_POSTCOPY; } if (multifd_recv_setup(errp) != 0) { return; } - if (default_channel) { + if (channel == CH_MAIN) { f = qemu_file_new_input(ioc); migration_incoming_setup(f); - } else { + } else if (channel == CH_MULTIFD) { /* Multiple connections */ - assert(migration_needs_multiple_sockets()); if (migrate_multifd()) { multifd_recv_new_channel(ioc, &local_err); - } else { - assert(migrate_postcopy_preempt()); - f = qemu_file_new_input(ioc); - postcopy_preempt_new_channel(mis, f); } if (local_err) { error_propagate(errp, local_err); return; } + } else if (channel == CH_POSTCOPY) { + assert(migrate_postcopy_preempt()); + assert(!mis->postcopy_qemufile_dst); + f = qemu_file_new_input(ioc); + postcopy_preempt_new_channel(mis, f); } - if (migration_should_start_incoming(default_channel)) { + if (channel != CH_POSTCOPY && migration_has_main_and_multifd_channels()) { /* If it's a recovery, we're done */ if (postcopy_try_recover()) { return; @@ -1058,20 +1067,15 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp) */ bool migration_has_all_channels(void) { + if (!migration_has_main_and_multifd_channels()) { + return false; + } + MigrationIncomingState *mis = migration_incoming_get_current(); - - if (!mis->from_src_file) { + if (migrate_postcopy_preempt() && !mis->postcopy_qemufile_dst) { return false; } - if (migrate_multifd()) { - return multifd_recv_all_channels_created(); - } - - if (migrate_postcopy_preempt()) { - return mis->postcopy_qemufile_dst != NULL; - } - return true; } @@ -1482,6 +1486,8 @@ static void migrate_fd_cleanup(MigrationState *s) assert(!migration_is_active()); + file_cleanup_outgoing_migration(); + socket_cleanup_outgoing_migration(); if (s->state == MIGRATION_STATUS_CANCELLING) { migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING, MIGRATION_STATUS_CANCELLED); @@ -3373,8 +3379,11 @@ static MigIterateState migration_iteration_run(MigrationState *s) } /* Still a significant amount to transfer */ - if (!in_postcopy && must_precopy <= s->threshold_size && can_switchover && - qatomic_read(&s->start_postcopy)) { + if (!in_postcopy && must_precopy <= s->threshold_size + && can_switchover && qatomic_read(&s->start_postcopy)) { + if (migrate_multifd()) { + multifd_send_shutdown(); + } if (postcopy_start(s, &local_err)) { migrate_set_error(s, local_err); error_report_err(local_err); diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c index 1325dba97c..d0edec7cd1 100644 --- a/migration/multifd-nocomp.c +++ b/migration/multifd-nocomp.c @@ -16,6 +16,7 @@ #include "file.h" #include "multifd.h" #include "options.h" +#include "migration.h" #include "qapi/error.h" #include "qemu/cutils.h" #include "qemu/error-report.h" @@ -391,7 +392,7 @@ int multifd_ram_flush_and_sync(QEMUFile *f) MultiFDSyncReq req; int ret; - if (!migrate_multifd()) { + if (!migrate_multifd() || migration_in_postcopy()) { return 0; } diff --git a/migration/multifd.c b/migration/multifd.c index 97ce831775..fa83a43778 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -467,8 +467,6 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) static void multifd_send_cleanup_state(void) { - file_cleanup_outgoing_migration(); - socket_cleanup_outgoing_migration(); qemu_sem_destroy(&multifd_send_state->channels_created); qemu_sem_destroy(&multifd_send_state->channels_ready); g_free(multifd_send_state->params); @@ -481,7 +479,7 @@ void multifd_send_shutdown(void) { int i; - if (!migrate_multifd()) { + if (!migrate_multifd() || !multifd_send_state) { return; } diff --git a/migration/options.c b/migration/options.c index 4db340b502..c4dfe89edd 100644 --- a/migration/options.c +++ b/migration/options.c @@ -480,11 +480,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp) error_setg(errp, "Postcopy is not compatible with ignore-shared"); return false; } - - if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) { - error_setg(errp, "Postcopy is not yet compatible with multifd"); - return false; - } } if (new_caps[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) { diff --git a/migration/ram.c b/migration/ram.c index 6f460fd22d..8f22745aba 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -1297,7 +1297,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss) pss->page = 0; pss->block = QLIST_NEXT_RCU(pss->block, next); if (!pss->block) { - if (multifd_ram_sync_per_round()) { + if (multifd_ram_sync_per_round() && !migration_in_postcopy()) { QEMUFile *f = rs->pss[RAM_CHANNEL_PRECOPY].pss_channel; int ret = multifd_ram_flush_and_sync(f); if (ret < 0) { @@ -1971,9 +1971,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss) } } - if (migrate_multifd()) { - RAMBlock *block = pss->block; - return ram_save_multifd_page(block, offset); + if (migrate_multifd() && !migration_in_postcopy()) { + return ram_save_multifd_page(pss->block, offset); } if (control_save_page(pss, offset, &res)) {