Re: [PATCH 3/3] qemu: support set parallel migration compression method

20 Apr 2023

On Fri, Feb 24, 2023 at 17:27:12 +0800, Jiang Jiacheng wrote:
...
Add new compress methods zlib and zstd for parallel migration,
these method should be used with migration option --comp-methods
and will be processed in 'qemuMigrationParamsSetCompression'.
Note that only one compress method could be chosen for parallel 
migration and they cann't be used in compress migration.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
---
 src/qemu/qemu_migration.h        |  2 +
 src/qemu/qemu_migration_params.c | 80 +++++++++++++++++++++++++++++++-
 src/qemu/qemu_migration_params.h |  3 ++
 3 files changed, 83 insertions(+), 2 deletions(-)
...
...

diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index bd09dcfb23..3c224131c5 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
...
@@ -504,8 +528,6 @@ qemuMigrationParamsSetTPString(qemuMigrationParams *migParams,
                                    migParams->params[param].value.s);
 }
-
-
Spurious whitespace change.
...
static int
 qemuMigrationParamsSetCompression(virTypedParameterPtr params,
                                   int nparams,
@@ -520,6 +542,13 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params,
         if (STRNEQ(params[i].field, VIR_MIGRATE_PARAM_COMPRESSION))
             continue;
+        if (migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].set) {
+            virReportError(VIR_ERR_INVALID_ARG, "%s",
+                           _("Only one compression method could be specified with "
+                             "parallel compression"));
The error message should all be on a single line. And I'd move this
check a bit further below the two new compatibility checks: [1].
...
+            return -1;
+        }
+
         method = qemuMigrationCompressMethodTypeFromString(params[i].value.s);
         if (method < 0) {
             virReportError(VIR_ERR_INVALID_ARG,
@@ -535,15 +564,43 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params,
             return -1;
         }
+        if ((method == QEMU_MIGRATION_COMPRESS_MT ||
+             method == QEMU_MIGRATION_COMPRESS_XBZRLE) &&
+            flags & VIR_MIGRATE_PARALLEL) {
+            virReportError(VIR_ERR_INVALID_ARG,
+                           _("Compression method '%s' isn't supported with parallel migration"),
Recent changes (made after you sent this patch) require %1$s format
string to be used instead of %s
...
+                           params[i].value.s);
+            return -1;
+        }
+
+        if ((method == QEMU_MIGRATION_COMPRESS_ZLIB ||
+             method == QEMU_MIGRATION_COMPRESS_ZSTD) &&
+            flags & VIR_MIGRATE_COMPRESSED) {
+            virReportError(VIR_ERR_INVALID_ARG,
+                           _("Compression method '%s' isn't supported with compress migration"),
+                           params[i].value.s);
+            return -1;
+        }
+
[1]
...
migParams->compMethods |= 1ULL << method;
switch ((qemuMigrationCompressMethod) method) {
         case QEMU_MIGRATION_COMPRESS_XBZRLE:
             cap = QEMU_MIGRATION_CAP_XBZRLE;
+            flags |= VIR_MIGRATE_COMPRESSED;
We do not magically enable flags based on other flags. We just report an
error if the required flag is not set.
...
break;
case QEMU_MIGRATION_COMPRESS_MT:
             cap = QEMU_MIGRATION_CAP_COMPRESS;
+            flags |= VIR_MIGRATE_COMPRESSED;
The same here.
...
+            break;
+
+        case QEMU_MIGRATION_COMPRESS_ZLIB:
+        case QEMU_MIGRATION_COMPRESS_ZSTD:
+            flags |= VIR_MIGRATE_PARALLEL;
+            cap = QEMU_MIGRATION_CAP_MULTIFD;
And same here (for both flags and cap);
...
+            migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].value.s = g_strdup(params[i].value.s);
+            migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].set = true;
             break;
case QEMU_MIGRATION_COMPRESS_LAST:
@@ -569,6 +626,20 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params,
         return -1;
     }
+    if (migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_ZLIB_LEVEL].set &&
+        !(migParams->compMethods & (1ULL << QEMU_MIGRATION_COMPRESS_ZLIB))) {
+        virReportError(VIR_ERR_INVALID_ARG, "%s",
+                       _("Turn zlib compression on to tune it"));
+        return -1;
+    }
+
+    if (migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_ZSTD_LEVEL].set &&
+        !(migParams->compMethods & (1ULL << QEMU_MIGRATION_COMPRESS_ZSTD))) {
+        virReportError(VIR_ERR_INVALID_ARG, "%s",
+                       _("Turn zstd compression on to tune it"));
+        return -1;
+    }
+
The idea was to consistently use

    --compressed --comp-method=... --comp-...-...

regardless on selected compression method or whether parallel migration
is turned on or not. Specifically,

    --parallel --compressed --comp-method=zlib --comp-zlib-...
    --parallel --compressed --comp-method=zstd --comp-zstd-...
    --compressed --comp-method=mt --comp-mt-...
    --compressed --comp-method=xbzrle,mt --comp-xbzrle-... --comp-mt-...
    --compressed

are all ok, while

    --compressed --comp-method=zlib
    --compressed --comp-method=zstd

should report an error about missing --parallel option and

    --parallel --compressed --comp-method=xbzrle
    --parallel --compressed --comp-method=mt

should report an error saying the selected method cannot be used with
parallel migration.

Actually looking at the current code (confirmed also by an experiment)
the --compressed parameter is not required. It's mostly a shortcut for a
default method, which is xbzrle for non-parallel migration. The default
for parallel migration is documented as "no compression", which would
mean

    --parallel --compressed

is equivalent to

    --parallel

I think it would be better to just forbid --compressed with --parallel
unless there is a compression method specified with --comp-method to
avoid a strange semantics of --compressed not providing any compression
at all.
...
if (!migParams->compMethods && (flags & VIR_MIGRATE_COMPRESSED)) {
         migParams->compMethods = 1ULL << QEMU_MIGRATION_COMPRESS_XBZRLE;
         ignore_value(virBitmapSetBit(migParams->caps,
@@ -690,6 +761,11 @@ qemuMigrationParamsDump(qemuMigrationParams *migParams,
         *flags |= VIR_MIGRATE_COMPRESSED;
     }
+    if (migParams->compMethods == 1ULL << QEMU_MIGRATION_COMPRESS_ZLIB ||
+        migParams->compMethods == 1ULL << QEMU_MIGRATION_COMPRESS_ZSTD) {
+        *flags |= VIR_MIGRATE_PARALLEL;
+    }
+
This is not needed as the VIR_MIGRATE_PARALLEL flag has to be set
explicitly.
...
for (i = 0; i < QEMU_MIGRATION_COMPRESS_LAST; ++i) {
         if ((migParams->compMethods & (1ULL << i)) &&
             virTypedParamsAddString(params, nparams, maxparams,
diff --git a/src/qemu/qemu_migration_params.h b/src/qemu/qemu_migration_params.h
index e7c65f6a21..5857673227 100644
--- a/src/qemu/qemu_migration_params.h
+++ b/src/qemu/qemu_migration_params.h
@@ -59,6 +59,9 @@ typedef enum {
     QEMU_MIGRATION_PARAM_XBZRLE_CACHE_SIZE,
     QEMU_MIGRATION_PARAM_MAX_POSTCOPY_BANDWIDTH,
     QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS,
+    QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION,
+    QEMU_MIGRATION_PARAM_MULTIFD_ZLIB_LEVEL,
+    QEMU_MIGRATION_PARAM_MULTIFD_ZSTD_LEVEL,
QEMU_MIGRATION_PARAM_LAST
 } qemuMigrationParam;
With the following suggested changes
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>

diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index ee23cec23d..807cccd86e 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -528,6 +528,8 @@ qemuMigrationParamsSetTPString(qemuMigrationParams *migParams,
                                    migParams->params[param].value.s);
 }

+
+
 static int
 qemuMigrationParamsSetCompression(virTypedParameterPtr params,
                                   int nparams,
@@ -536,19 +538,11 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params,
 {
     size_t i;
     int method;
-    qemuMigrationCapability cap;

     for (i = 0; i < nparams; i++) {
         if (STRNEQ(params[i].field, VIR_MIGRATE_PARAM_COMPRESSION))
             continue;

-        if (migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].set) {
-            virReportError(VIR_ERR_INVALID_ARG, "%s",
-                           _("Only one compression method could be specified with "
-                             "parallel compression"));
-            return -1;
-        }
-
         method = qemuMigrationCompressMethodTypeFromString(params[i].value.s);
         if (method < 0) {
             virReportError(VIR_ERR_INVALID_ARG,
@@ -568,46 +562,47 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params,
              method == QEMU_MIGRATION_COMPRESS_XBZRLE) &&
             flags & VIR_MIGRATE_PARALLEL) {
             virReportError(VIR_ERR_INVALID_ARG,
-                           _("Compression method '%s' isn't supported with parallel migration"),
+                           _("Compression method '%1$s' isn't supported with parallel migration"),
                            params[i].value.s);
             return -1;
         }

         if ((method == QEMU_MIGRATION_COMPRESS_ZLIB ||
              method == QEMU_MIGRATION_COMPRESS_ZSTD) &&
-            flags & VIR_MIGRATE_COMPRESSED) {
+            !(flags & VIR_MIGRATE_PARALLEL)) {
             virReportError(VIR_ERR_INVALID_ARG,
-                           _("Compression method '%s' isn't supported with compress migration"),
+                           _("Compression method '%1$s' is only supported with parallel migration"),
                            params[i].value.s);
             return -1;
         }

+        if (migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].set) {
+            virReportError(VIR_ERR_INVALID_ARG, "%s",
+                           _("Only one compression method could be specified with parallel compression"));
+            return -1;
+        }
+
         migParams->compMethods |= 1ULL << method;

         switch ((qemuMigrationCompressMethod) method) {
         case QEMU_MIGRATION_COMPRESS_XBZRLE:
-            cap = QEMU_MIGRATION_CAP_XBZRLE;
-            flags |= VIR_MIGRATE_COMPRESSED;
+            ignore_value(virBitmapSetBit(migParams->caps, QEMU_MIGRATION_CAP_XBZRLE));
             break;

         case QEMU_MIGRATION_COMPRESS_MT:
-            cap = QEMU_MIGRATION_CAP_COMPRESS;
-            flags |= VIR_MIGRATE_COMPRESSED;
+            ignore_value(virBitmapSetBit(migParams->caps, QEMU_MIGRATION_CAP_COMPRESS));
             break;

         case QEMU_MIGRATION_COMPRESS_ZLIB:
         case QEMU_MIGRATION_COMPRESS_ZSTD:
-            flags |= VIR_MIGRATE_PARALLEL;
-            cap = QEMU_MIGRATION_CAP_MULTIFD;
             migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].value.s = g_strdup(params[i].value.s);
             migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].set = true;
             break;

         case QEMU_MIGRATION_COMPRESS_LAST:
         default:
-            continue;
+            break;
         }
-        ignore_value(virBitmapSetBit(migParams->caps, cap));
     }

     if ((migParams->params[QEMU_MIGRATION_PARAM_COMPRESS_LEVEL].set ||
@@ -641,6 +636,12 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params,
     }

     if (!migParams->compMethods && (flags & VIR_MIGRATE_COMPRESSED)) {
+        if (flags & VIR_MIGRATE_PARALLEL) {
+            virReportError(VIR_ERR_INVALID_ARG, "%s",
+                           _("No compression algorithm selected for parallel migration"));
+            return -1;
+        }
+
         migParams->compMethods = 1ULL << QEMU_MIGRATION_COMPRESS_XBZRLE;
         ignore_value(virBitmapSetBit(migParams->caps,
                                      QEMU_MIGRATION_CAP_XBZRLE));
@@ -761,11 +762,6 @@ qemuMigrationParamsDump(qemuMigrationParams *migParams,
         *flags |= VIR_MIGRATE_COMPRESSED;
     }

-    if (migParams->compMethods == 1ULL << QEMU_MIGRATION_COMPRESS_ZLIB ||
-        migParams->compMethods == 1ULL << QEMU_MIGRATION_COMPRESS_ZSTD) {
-        *flags |= VIR_MIGRATE_PARALLEL;
-    }
-
     for (i = 0; i < QEMU_MIGRATION_COMPRESS_LAST; ++i) {
         if ((migParams->compMethods & (1ULL << i)) &&
             virTypedParamsAddString(params, nparams, maxparams,

    