[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/2] migration: enfocre multifd and postcopy preempt to be
From: |
Juan Quintela |
Subject: |
Re: [PATCH v2 1/2] migration: enfocre multifd and postcopy preempt to be set before incoming |
Date: |
Wed, 21 Jun 2023 22:34:54 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
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.