On Fri, Feb 24, 2023 at 17:27:10 +0800, Jiang Jiacheng wrote:

Add description for VIR_MIGRATE_PARAM_COMPRESSION, it will be reused in choosing compression method during parallel migration. Add public API VIR_MIGRATE_PARAM_COMPRESSION_ZLIB_LEVEL, VIR_MIGRATE_PARAM_COMPRESSION_ZSTD_LEVEL for migration APIs to support set compress level during parallel migration.

Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- include/libvirt/libvirt-domain.h | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 5152ed4551..40b6b19495 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1269,9 +1269,11 @@ typedef enum { * VIR_MIGRATE_PARAM_COMPRESSION: * * virDomainMigrate* params multiple field: name of the method used to - * compress migration traffic. Supported compression methods: xbzrle, mt. - * The parameter may be specified multiple times if more than one method - * should be used. + * compress migration traffic. Supported compression methods: xbzrle, mt, + * zlib, zstd. The parameter may be specified multiple times if more than + * one method. Not all combinations of compression methods and migration

Looks like something is missing after "if more than one method" here. Perhaps "if more than one method should be used"?

+ * options may be allowed. Parallel migration of QEMU domains is only + * compatible with either zlib or zstd method. * * Since: 1.3.4 */ ...

With the following suggested changes Reviewed-by: Jiri Denemark <jdenemar@redhat.com> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 46b10fd908..984ae8883b 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1272,9 +1272,9 @@ typedef enum { * virDomainMigrate* params multiple field: name of the method used to * compress migration traffic. Supported compression methods: xbzrle, mt, * zlib, zstd. The parameter may be specified multiple times if more than - * one method. Not all combinations of compression methods and migration - * options may be allowed. Parallel migration of QEMU domains is only - * compatible with either zlib or zstd method. + * one method should be used. Not all combinations of compression methods + * and migration options may be allowed. Parallel migration of QEMU domains + * is only compatible with either zlib or zstd method. * * Since: 1.3.4 */

On Fri, Feb 24, 2023 at 17:27:11 +0800, Jiang Jiacheng wrote:

Add migrate options: --compression-zlib-level --compression-zstd-level These options are used to set compress level for "zlib" or "zstd" during parallel migration if the compress method is specified.

Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- docs/manpages/virsh.rst | 29 ++++++++++++++++++++--------- tools/virsh-domain.c | 26 ++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index d5b614dc03..ccad02bd94 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -3356,7 +3356,8 @@ migrate [--xml file] [--migrate-disks disk-list] [--disks-port port] [--compressed] [--comp-methods method-list] [--comp-mt-level] [--comp-mt-threads] [--comp-mt-dthreads] - [--comp-xbzrle-cache] [--auto-converge] [auto-converge-initial] + [--comp-xbzrle-cache] [--comp-zlib-level] [--comp-zstd-level] + [--auto-converge] [auto-converge-initial] [auto-converge-increment] [--persistent-xml file] [--tls] [--postcopy-bandwidth bandwidth] [--parallel [--parallel-connections connections]] @@ -3463,14 +3464,24 @@ to post-copy upon timeout; migration has to be started with *--postcopy* option for this to work.

*--compressed* activates compression, the compression method is chosen -with *--comp-methods*. Supported methods are "mt" and "xbzrle" and -can be used in any combination. When no methods are specified, a hypervisor -default methods will be used. QEMU defaults to "xbzrle". Compression methods -can be tuned further. *--comp-mt-level* sets compression level. -Values are in range from 0 to 9, where 1 is maximum speed and 9 is maximum -compression. *--comp-mt-threads* and *--comp-mt-dthreads* set the number -of compress threads on source and the number of decompress threads on target -respectively. *--comp-xbzrle-cache* sets size of page cache in bytes. +with *--comp-methods*. Supported methods are "mt", "xbzrle", "zlib", +and "zstd". The supported set of methods and their combinations depend +on a hypervisor and migration options. QEMU only supports "zlib" and +"zstd" methods when *--parallel* is used and they cannot be used at +once. When no methods are specified, a hypervisor default methods will +be used. QEMU defaults to no compression for *--parallel* migration and +"xbzrle" otherwise. Compression methods can be tuned further.

This would suggest --parallel --compressed is allowed and results in no compression as that is the default for QEMU. I think it would be better to explicitly say that using --compressed without --comp-method is not allowed with --parallel.

+*--comp-mt-level* sets compression level for "mt" method. Values are in +range from 0 to 9, where 1 is maximum speed and 9 is maximum compression. +*--comp-mt-threads* and *--comp-mt-dthreads* set the number of compress +threads on source and the number of decompress threads on target respectively. +*--comp-xbzrle-cache* sets size of page cache in bytes. +*--comp-zlib-level* sets the compression level when using "zlib" method. +Values are in range from 0 to 9 and defaults to 1, where 0 is no compression, +1 is maximum speed and 9 is maximum compression. +*--comp-zstd-level* sets the compression level when using "zstd" method. +Values are in range from 0 to 20 and defaults to 1, where 0 is no compression, +1 is maximum speed and 20 is maximum compression.

Providing *--tls* causes the migration to use the host configured TLS setup (see migrate_tls_x509_cert_dir in /etc/libvirt/qemu.conf) in order to perform ...

With the following suggested changes Reviewed-by: Jiri Denemark <jdenemar@redhat.com> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 738d3b7fb9..f4e5a0bd62 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -3485,19 +3485,20 @@ and "zstd". The supported set of methods and their combinations depend on a hypervisor and migration options. QEMU only supports "zlib" and "zstd" methods when *--parallel* is used and they cannot be used at once. When no methods are specified, a hypervisor default methods will -be used. QEMU defaults to no compression for *--parallel* migration and -"xbzrle" otherwise. Compression methods can be tuned further. -*--comp-mt-level* sets compression level for "mt" method. Values are in -range from 0 to 9, where 1 is maximum speed and 9 is maximum compression. -*--comp-mt-threads* and *--comp-mt-dthreads* set the number of compress -threads on source and the number of decompress threads on target respectively. -*--comp-xbzrle-cache* sets size of page cache in bytes. -*--comp-zlib-level* sets the compression level when using "zlib" method. -Values are in range from 0 to 9 and defaults to 1, where 0 is no compression, -1 is maximum speed and 9 is maximum compression. -*--comp-zstd-level* sets the compression level when using "zstd" method. -Values are in range from 0 to 20 and defaults to 1, where 0 is no compression, -1 is maximum speed and 20 is maximum compression. +be used. QEMU defaults to "xbzrle" as long as *--parallel* is not used. +For *--parallel* migrations QEMU does not provide any default compression +method and thus it has to be specified explicitly using *--comp-method*. +Compression methods can be tuned further. *--comp-mt-level* sets +compression level for "mt" method. Values are in range from 0 to 9, where 1 +is maximum speed and 9 is maximum compression. *--comp-mt-threads* and +*--comp-mt-dthreads* set the number of compress threads on source and the +number of decompress threads on target respectively. *--comp-xbzrle-cache* +sets size of page cache in bytes. *--comp-zlib-level* sets the compression +level when using "zlib" method. Values are in range from 0 to 9 and defaults +to 1, where 0 is no compression, 1 is maximum speed and 9 is maximum +compression. *--comp-zstd-level* sets the compression level when using "zstd" +method. Values are in range from 0 to 20 and defaults to 1, where 0 is no +compression, 1 is maximum speed and 20 is maximum compression. Providing *--tls* causes the migration to use the host configured TLS setup (see migrate_tls_x509_cert_dir in /etc/libvirt/qemu.conf) in order to perform

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,