Re: [PATCH v2 1/2] migration: enfocre multifd and postcopy preempt to be set before incoming

[Juan Quintela <quintela@redhat.com>]

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.