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

[PATCH 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.

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>
---
 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 v2 2/2] qtest/migration-tests.c: use "-incoming defer" for postcopy tests

[Wei Wang]

The Postcopy preempt capability requires 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.

Signed-off-by: Wei Wang <wei.w.wang@intel.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 b99b49a314..31ce3d959c 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1158,10 +1158,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;
     }
 
@@ -1180,9 +1181,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 2/2] qtest/migration-tests.c: use "-incoming defer" for postcopy tests

[Peter Xu]

On Tue, May 30, 2023 at 05:02:59PM +0800, Wei Wang wrote:
> The Postcopy preempt capability requires 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.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>

Hmm.. so we used to do socket_start_incoming_migration_internal() before
setting the right num for the preempt test, then I'm curious why it wasn't
failing before this patch when trying to connect with the preempt channel..

Wei, do you know?

> ---
>  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 b99b49a314..31ce3d959c 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1158,10 +1158,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;
>      }
>  
> @@ -1180,9 +1181,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
> 

-- 
Peter Xu

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

[Wang, Wei W]

On Tuesday, May 30, 2023 10:41 PM, Peter Xu wrote:
> On Tue, May 30, 2023 at 05:02:59PM +0800, Wei Wang wrote:
> > The Postcopy preempt capability requires 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.
> >
> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> 
> Hmm.. so we used to do socket_start_incoming_migration_internal() before
> setting the right num for the preempt test, then I'm curious why it wasn't
> failing before this patch when trying to connect with the preempt channel..
> 
> Wei, do you know?

I think there are 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. (This is easier to trigger for the multifd case, e.g. use a much
larger number like 16).
#2 per my tests (on kernel 6.2), the number of pending connections allowed is actually
"backlog + 1", which is 2 in this case. Adding more pending connections will then be
dropped. I'm not sure if " backlog + 1" is true for older versions of kernel, though.

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

[Peter Xu]

On Wed, May 31, 2023 at 10:41:37AM +0000, Wang, Wei W wrote:
> On Tuesday, May 30, 2023 10:41 PM, Peter Xu wrote:
> > On Tue, May 30, 2023 at 05:02:59PM +0800, Wei Wang wrote:
> > > The Postcopy preempt capability requires 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.
> > >
> > > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > 
> > Hmm.. so we used to do socket_start_incoming_migration_internal() before
> > setting the right num for the preempt test, then I'm curious why it wasn't
> > failing before this patch when trying to connect with the preempt channel..
> > 
> > Wei, do you know?
> 
> I think there are 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. (This is easier to trigger for the multifd case, e.g. use a much
> larger number like 16).
> #2 per my tests (on kernel 6.2), the number of pending connections allowed is actually
> "backlog + 1", which is 2 in this case. Adding more pending connections will then be
> dropped. I'm not sure if " backlog + 1" is true for older versions of kernel, though.

Interesting to know, thanks.

If there'll be a new version, please consider adding some of those into the
commit message.

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

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

[Wang, Wei W]

On Wednesday, May 31, 2023 8:58 PM, Peter Xu wrote:
> > > Hmm.. so we used to do socket_start_incoming_migration_internal()
> > > before setting the right num for the preempt test, then I'm curious
> > > why it wasn't failing before this patch when trying to connect with the
> preempt channel..
> > >
> > > Wei, do you know?
> >
> > I think there are 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. (This
> > is easier to trigger for the multifd case, e.g. use a much larger number like
> 16).
> > #2 per my tests (on kernel 6.2), the number of pending connections
> > allowed is actually "backlog + 1", which is 2 in this case. Adding
> > more pending connections will then be dropped. I'm not sure if " backlog +
> 1" is true for older versions of kernel, though.
> 
> Interesting to know, thanks.
> 
> If there'll be a new version, please consider adding some of those into the
> commit message.

OK, will resend with commit update.
Plan to wait a bit in case there would be other feedbacks.

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