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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
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,
