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

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

[Wei Wang]

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>
---
 migration/options.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index a560483871..954a39aa6d 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 &&
-- 
2.27.0

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

[Peter Xu]

On Tue, May 30, 2023 at 05:02:58PM +0800, Wei Wang 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>

-- 
Peter Xu

[PATCH RESEND v2 0/2] Enfore multifd and postcopy preempt setting

[Wei Wang]

Setting the cap/params of multifd and postcopy preempt is expected
to be done before incoming starts, as the number of multifd channels or
postcopy ram channels is used by qemu_start_incoming_migration (to
listen on the number of pending connections).

Enfocre the cap/params of multifd and postcopy preempt to be set
before incoming. If not, disable the feature and return with error
messages to remind users to adjust the commands. Fix the
qtest/migration-test to do so.

v2 RESEND change:
- Only patch commit change with more explanations

Wei Wang (2):
  migration: enfocre multifd and postcopy preempt to be set before
    incoming
  qtest/migration-tests.c: use "-incoming defer" for postcopy tests

 migration/options.c          | 36 +++++++++++++++++++++++++++++++-----
 tests/qtest/migration-test.c | 10 ++++++++--
 2 files changed, 39 insertions(+), 7 deletions(-)

-- 
2.27.0

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

[Wei Wang]

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(-)

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 &&
-- 
2.27.0

[PATCH v2 2/2] qtest/migration-tests.c: use "-incoming defer" for postcopy tests

[Wei Wang]

The Postcopy preempt capability is expected to be set before incoming
starts, so change the postcopy tests to start with deferred incoming and
call migrate-incoming after the cap has been set.

Why the existing tests (without this patch) didn't fail?
There could be two reasons:
1) "backlog" specifies the number of pending connections. As long as the
   server accepts the connections faster than the clients side connecting,
   connection will succeed. For the preempt test, it uses only 2 channels,
   so very likely to not have pending connections.
2) per my tests (on kernel 6.2), the number of pending connections allowed
   is actually "backlog + 1", which is 2 in this case.
That said, the implementation of socket_start_incoming_migration_internal
expects "migrate defer" to be used, and for safety, change the test to
work with the expected usage.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 tests/qtest/migration-test.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index b0c355bbd9..cbdbc932de 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1143,10 +1143,11 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
                                     QTestState **to_ptr,
                                     MigrateCommon *args)
 {
-    g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
+    g_autofree char *uri = NULL;
     QTestState *from, *to;
+    QDict *rsp;
 
-    if (test_migrate_start(&from, &to, uri, &args->start)) {
+    if (test_migrate_start(&from, &to, "defer", &args->start)) {
         return -1;
     }
 
@@ -1165,9 +1166,14 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
 
     migrate_ensure_non_converge(from);
 
+    rsp = wait_command(to, "{ 'execute': 'migrate-incoming',"
+                           "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
+    qobject_unref(rsp);
+
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
 
+    uri = migrate_get_socket_address(to, "socket-address");
     migrate_qmp(from, uri, "{}");
 
     wait_for_migration_pass(from);
-- 
2.27.0

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

[Juan Quintela]

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.

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

[Juan Quintela]

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.