| Message ID | 20230606101910.20456-2-wei.w.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enfore multifd and postcopy preempt setting | expand |
Wei Wang <wei.w.wang@intel.com> wrote: > qemu_start_incoming_migration needs to check the number of multifd > channels or postcopy ram channels to configure the backlog parameter (i.e. > the maximum length to which the queue of pending connections for sockfd > may grow) of listen(). So enforce the usage of postcopy-preempt and > multifd as below: > - need to use "-incoming defer" on the destination; and > - set_capability and set_parameter need to be done before migrate_incoming > > Otherwise, disable the use of the features and report error messages to > remind users to adjust the commands. > > Signed-off-by: Wei Wang <wei.w.wang@intel.com> > Reviewed-by: Peter Xu <peterx@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> queued.
Wei Wang <wei.w.wang@intel.com> wrote: > qemu_start_incoming_migration needs to check the number of multifd > channels or postcopy ram channels to configure the backlog parameter (i.e. > the maximum length to which the queue of pending connections for sockfd > may grow) of listen(). So enforce the usage of postcopy-preempt and > multifd as below: > - need to use "-incoming defer" on the destination; and > - set_capability and set_parameter need to be done before migrate_incoming > > Otherwise, disable the use of the features and report error messages to > remind users to adjust the commands. > > Signed-off-by: Wei Wang <wei.w.wang@intel.com> > Reviewed-by: Peter Xu <peterx@redhat.com> > --- > migration/options.c | 36 +++++++++++++++++++++++++++++++----- > 1 file changed, 31 insertions(+), 5 deletions(-) > This bit is wrong > @@ -998,11 +1013,22 @@ bool migrate_params_check(MigrationParameters *params, Error **errp) > > /* x_checkpoint_delay is now always positive */ > > - if (params->has_multifd_channels && (params->multifd_channels < 1)) { > - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, > - "multifd_channels", > - "a value between 1 and 255"); > - return false; > + if (params->has_multifd_channels) { > + if (params->multifd_channels < 1) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, > + "multifd_channels", > + "a value between 1 and 255"); > + return false; > + } > + if (migrate_incoming_started()) { > + MigrationState *ms = migrate_get_current(); > + > + ms->capabilities[MIGRATION_CAPABILITY_MULTIFD] = false; > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, > + "multifd_channels", > + "must be set before incoming starts"); > + return false; > + } > } > > if (params->has_multifd_zlib_level && # Start of tls tests # starting QEMU: exec ./qemu-system-x86_64 -qtest unix:/tmp/qtest-3655124.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3655124.qmp,id=char0 -mon chardev=char0,mode=control -display none -accel kvm -accel tcg -name source,debug-threads=on -m 150M -serial file:/tmp/migration-test-5QEX61/src_serial -drive file=/tmp/migration-test-5QEX61/bootsect,format=raw 2>/dev/null -accel qtest # starting QEMU: exec ./qemu-system-x86_64 -qtest unix:/tmp/qtest-3655124.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3655124.qmp,id=char0 -mon chardev=char0,mode=control -display none -accel kvm -accel tcg -name target,debug-threads=on -m 150M -serial file:/tmp/migration-test-5QEX61/dest_serial -incoming unix:/tmp/migration-test-5QEX61/migsocket -drive file=/tmp/migration-test-5QEX61/bootsect,format=raw 2>/dev/null -accel qtest # { # "error": { # "class": "GenericError", # "desc": "Parameter 'multifd_channels' expects must be set before incoming starts" # } # } ** ERROR:../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:1259:qtest_vqmp_assert_success_ref: assertion failed: (qdict_haskey(response, "return")) not ok /x86_64/migration/postcopy/recovery/tls/psk - ERROR:../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:1259:qtest_vqmp_assert_success_ref: assertion failed: (qdict_haskey(response, "return")) Bail out! Aborted (core dumped) This is the tests that fails. qtest_add_func("/migration/postcopy/preempt/plain", test_postcopy_preempt); I am dropping that change and let the others, which are right. I think what we should do is changing that check to: static bool migration_has_started(void) { MigrationIncomingState *mis = migration_incoming_get_current(); if (mis->state != MIGRATION_STATUS_NONE) { return true; } MigrationState *ms = migration_get_current(); if (mis->state != MIGRATION_STATUS_NONE) { return true; } return false; } And for all the parameters that can't be changed after migration has started do: if (params->has_multifd_channels) { if (params->multifd_channels < 1) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_channels", "a value between 1 and 255"); return false; } if (migration_has_started()) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_channels", "must be set before migration starts"); return false; } } Forr all parameters, can they be changed after migration has started: compress_level: NO compress_threads: NO compress_wait_thread: NO decompress_threads: NO throttle_trigger_threshold: MAYBE cpu_throttle_initial: NO cpu_throttle_increment: NO? cpu_throotle_tailslow: NO? tls_creds: NO tls_hostname: NO max_bandwidth: YES downtime_limit: YES x_checkpoint_delay: NO? block_incremental: NO multifd_channels: NO multifd_compression: NO multifd_zlib_devel: NO multifd_zstd_level: NO xbzrle_cache_size: YES max_cpu_throttle: YES? announce_*: YES (but it shouldn't. There is no good reason for changing it in the middle of migration. But no harm either) block_bitmap_mapping: NO? zero_copy_send: NO vcpu_dirty_limit_period: NO? vcpu_dirty_limit: NO? For capabilities, I think none make sense to change after migration starts. What do you think? Later, Juan.
diff --git a/migration/options.c b/migration/options.c index b62ab30cd5..01403e5eaa 100644 --- a/migration/options.c +++ b/migration/options.c @@ -415,6 +415,11 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot, MIGRATION_CAPABILITY_VALIDATE_UUID, MIGRATION_CAPABILITY_ZERO_COPY_SEND); +static bool migrate_incoming_started(void) +{ + return !!migration_incoming_get_current()->transport_data; +} + /** * @migration_caps_check - check capability compatibility * @@ -538,6 +543,12 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp) error_setg(errp, "Postcopy preempt not compatible with compress"); return false; } + + if (migrate_incoming_started()) { + error_setg(errp, + "Postcopy preempt must be set before incoming starts"); + return false; + } } if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) { @@ -545,6 +556,10 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp) error_setg(errp, "Multifd is not compatible with compress"); return false; } + if (migrate_incoming_started()) { + error_setg(errp, "Multifd must be set before incoming starts"); + return false; + } } return true; @@ -998,11 +1013,22 @@ bool migrate_params_check(MigrationParameters *params, Error **errp) /* x_checkpoint_delay is now always positive */ - if (params->has_multifd_channels && (params->multifd_channels < 1)) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - "multifd_channels", - "a value between 1 and 255"); - return false; + if (params->has_multifd_channels) { + if (params->multifd_channels < 1) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, + "multifd_channels", + "a value between 1 and 255"); + return false; + } + if (migrate_incoming_started()) { + MigrationState *ms = migrate_get_current(); + + ms->capabilities[MIGRATION_CAPABILITY_MULTIFD] = false; + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, + "multifd_channels", + "must be set before incoming starts"); + return false; + } } if (params->has_multifd_zlib_level &&