On Fri, Jan 20, 2023 at 16:47:40 +0800, Jiang Jiacheng wrote:

Add description for VIR_MIGRATE_PARAM_COMPRESSION, it will be reused in choosing compression method during multifd 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 multifd migration.

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

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 5152ed4551..deac3687ed 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1271,7 +1271,9 @@ typedef enum { * 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. + * should be used. Support compression methods for multifd migration: zlib, + * zstd. When using with multifd migration, only one supported compression + * method can be specified.

I don't think we should separate the new methods this way. And we use "parallel" instead of "multifd" migration. So how about: 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 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 */ @@ -1351,6 +1353,26 @@ typedef enum { */ # define VIR_MIGRATE_PARAM_PARALLEL_CONNECTIONS "parallel.connections"

+/** + * VIR_MIGRATE_PARAM_COMPRESSION_ZLIB_LEVEL: + * + * virDomainMigrate* params field: zlib compress level used during parallel + * migration. As VIR_TYPED_PARAM_INT.

I don't think we need to explicitly mention parallel migration here. On the other hand, we should mention allowed values and their semantics. See how compression.mt.level is described: "Accepted values are in range 0-9. 0 is no compression, 1 is maximum speed and 9 is maximum compression."

+ * + * Since: 9.1.0 + */ +# define VIR_MIGRATE_PARAM_COMPRESSION_ZLIB_LEVEL "compression.zlib.level" + +/** + * VIR_MIGRATE_PARAM_COMPRESSION_ZSTD_LEVEL: + * + * virDomainMigrate* params field: zstd compress level used during parallel + * migration. As VIR_TYPED_PARAM_INT.

The same comment as above.

+ * + * Since: 9.1.0 + */ +# define VIR_MIGRATE_PARAM_COMPRESSION_ZSTD_LEVEL "compression.zstd.level" +

And I think documenting them next to the other VIR_MIGRATE_PARAM_COMPRESSION parameters would make more sense.

/** * VIR_MIGRATE_PARAM_TLS_DESTINATION: *

Jirka

On 2023/2/7 23:37, Jiri Denemark wrote:

On Fri, Jan 20, 2023 at 16:47:41 +0800, Jiang Jiacheng wrote:

...

Add qemu migration parameters to support setting multifd migration compression method and level.

Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_migration.h | 2 ++ src/qemu/qemu_migration_params.c | 24 +++++++++++++++++++++++- src/qemu/qemu_migration_params.h | 3 +++ 3 files changed, 28 insertions(+), 1 deletion(-)

...

...

diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index bd09dcfb23..380fc7dccd 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c ... @@ -662,7 +682,9 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params, return NULL; }

- if (migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS].set && + if ((migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS].set || + migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_ZLIB_LEVEL].set || + migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_ZSTD_LEVEL].set) && !(flags & VIR_MIGRATE_PARALLEL)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Turn parallel migration on to tune it"));

This check doesn't really make sense here. The new parameters tune multifd compression (if enabled) rather than multifd migration itself.

...

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;

And this patch should actually be squashed into the next one.

Thanks for your suggestion, I will improve them in my next version. Thanks, Jiang Jiacheng

Jirka

On Fri, Jan 20, 2023 at 16:47:42 +0800, Jiang Jiacheng wrote:

Add 'qemuMigrationParamsSetParallelCompression' to support set parallel migration compression method. Depending on whether '--parallel' is set, we invoke different functions to select compression method from the same param 'VIR_MIGRATE_PARAM_COMPRESSION'.

Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_migration_params.c | 76 +++++++++++++++++++++++++++++++- 1 file changed, 74 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 380fc7dccd..4980761712 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -71,6 +71,8 @@ struct _qemuMigrationParams { typedef enum { QEMU_MIGRATION_COMPRESS_XBZRLE = 0, QEMU_MIGRATION_COMPRESS_MT, + QEMU_MIGRATION_COMPRESS_ZLIB, + QEMU_MIGRATION_COMPRESS_ZSTD,

QEMU_MIGRATION_COMPRESS_LAST } qemuMigrationCompressMethod; @@ -79,6 +81,8 @@ VIR_ENUM_IMPL(qemuMigrationCompressMethod, QEMU_MIGRATION_COMPRESS_LAST, "xbzrle", "mt", + "zlib", + "zstd", );

VIR_ENUM_IMPL(qemuMigrationCapability, @@ -524,6 +528,60 @@ qemuMigrationParamsSetTPString(qemuMigrationParams *migParams, migParams->params[param].value.s); }

+static int +qemuMigrationParamsSetParallelCompression(virTypedParameterPtr params, + int nparams, + qemuMigrationParams *migParams) +{ + size_t i; + int method; + const char *value = NULL; + int rc; +

This function is very similar to qemuMigrationParamsSetCompression and you need to handle the same values in both functions. Merge them into a single one that would handle all compression methods and whether they are allowed or not depending on the API flags.

+ 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 " + "multifd compression")); + return -1; + } + + method = qemuMigrationCompressMethodTypeFromString(params[i].value.s); + if (method < 2) {

Even though method is declared as int, it is an enum in reality. We store it in int because the API for converting a string to the corresponding enum value can return -1. Thus you should always use enum items in comparisons rather than magic integer values. And exact checks are much better than < or > as they don't break when new method gets introduced. And I think it's better to let the for loop translate VIR_MIGRATE_PARAM_COMPRESSION into migParams->compMethods and do the checks afterwards.

+ virReportError(VIR_ERR_INVALID_ARG, + _("Unsupported compression method '%s' with multifd migration"), + params[i].value.s); + return -1; + } + + migParams->compMethods |= 1ULL << method; + + if ((rc = virTypedParamsGetString(params, nparams, + VIR_MIGRATE_PARAM_COMPRESSION, &value)) < 0) + return -1; + + migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].value.s = g_strdup(value); + migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].set = !!rc; + } + + 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; + } + + return 0; +}

static int @@ -548,6 +606,13 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params, return -1; }

+ if (method > 1) { + virReportError(VIR_ERR_INVALID_ARG, + _("Trun parallel migration on to use compression method '%s'"), + params[i].value.s); + return -1; + } + if (migParams->compMethods & (1ULL << method)) { virReportError(VIR_ERR_INVALID_ARG, _("Compression method '%s' is specified twice"), @@ -566,6 +631,8 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params, cap = QEMU_MIGRATION_CAP_COMPRESS; break;

+ case QEMU_MIGRATION_COMPRESS_ZLIB: + case QEMU_MIGRATION_COMPRESS_ZSTD: case QEMU_MIGRATION_COMPRESS_LAST: default: continue; @@ -691,8 +758,13 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params, return NULL; }

- if (qemuMigrationParamsSetCompression(params, nparams, flags, migParams) < 0) - return NULL; + if (flags & VIR_MIGRATE_PARALLEL) { + if (qemuMigrationParamsSetParallelCompression(params, nparams, migParams) < 0) + return NULL; + } else { + if (qemuMigrationParamsSetCompression(params, nparams, flags, migParams) < 0) + return NULL; + }

As I said just handle all compression methods and options in the existing qemuMigrationParamsSetCompression function.

return g_steal_pointer(&migParams); }

Jirka

On 2023/1/20 17:41, Claudio Fontana wrote:

On 1/20/23 09:47, 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 | 22 +++++++++++++++------- tools/virsh-domain.c | 26 ++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index d5b614dc03..a837e3c1af 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -3359,7 +3359,8 @@ migrate [--comp-xbzrle-cache] [--auto-converge] [auto-converge-initial] [auto-converge-increment] [--persistent-xml file] [--tls] [--postcopy-bandwidth bandwidth] - [--parallel [--parallel-connections connections]] + [--parallel [--parallel-connections connections]

Hi, why the removal of the ] at the end of line ?

I move it to the back of '--comp-zstd-level', since they are options of parallel migration and should be set only when '--parallel' is chosen. Thanks Jiang Jiacheng

Thanks, C

...

+ [--comp-zlib-level][--comp-zstd-level]] [--bandwidth bandwidth] [--tls-destination hostname] [--disks-uri URI] [--copy-storage-synchronous-writes]

@@ -3466,11 +3467,14 @@ option for this to work. 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. +can be tuned further. When used with --parallel, supported methods are "zlib" +and "zstd" and if no methods are specified, QEMU will deaults to "none". +Note that compression method is defferent from compress migration and multifd +migration. *--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.

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 @@ -3486,7 +3490,11 @@ starting the migration. parallel connections. The number of such connections can be set using *--parallel-connections*. Parallel connections may help with saturating the network link between the source and the target and thus speeding up the -migration. +migration. *--comp-zlib-level* sets the compression level when using "zlib" +as multifd migration's compression mesthod. Values are in range from 0 to 9 +and defaults to 1. *--comp-zstd-level* sets the compression level when +using "zstd" as multifd migration's compression mesthod. Values are in range +from 0 to 20 and defaults to 1.

Running migration can be canceled by interrupting virsh (usually using ``Ctrl-C``) or by ``domjobabort`` command sent from another virsh instance. diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6b431bd1e5..bcf03e9567 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11095,6 +11095,14 @@ static const vshCmdOptDef opts_migrate[] = { .type = VSH_OT_INT, .help = N_("number of connections for parallel migration") }, + {.name = "comp-zlib-level", + .type = VSH_OT_INT, + .help = N_("zlib compression level used in parallel migration") + }, + {.name = "comp-zstd-level", + .type = VSH_OT_INT, + .help = N_("zstd compression level used in parallel migration") + }, {.name = "bandwidth", .type = VSH_OT_INT, .help = N_("migration bandwidth limit in MiB/s") @@ -11326,6 +11334,24 @@ doMigrate(void *opaque) goto save_error; }

+ if ((rv = vshCommandOptInt(ctl, cmd, "comp-zlib-level", &intOpt)) < 0) { + goto out; + } else if (rv > 0) { + if (virTypedParamsAddInt(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_COMPRESSION_ZLIB_LEVEL, + intOpt) < 0) + goto save_error; + } + + if ((rv = vshCommandOptInt(ctl, cmd, "comp-zstd-level", &intOpt)) < 0) { + goto out; + } else if (rv > 0) { + if (virTypedParamsAddInt(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_COMPRESSION_ZSTD_LEVEL, + intOpt) < 0) + goto save_error; + } + if ((rv = vshCommandOptULongLong(ctl, cmd, "bandwidth", &ullOpt)) < 0) { goto out; } else if (rv > 0) {

On Fri, Jan 20, 2023 at 16:47:43 +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 | 22 +++++++++++++++------- tools/virsh-domain.c | 26 ++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-)

I'll have similar comments to what I said for the public API.

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index d5b614dc03..a837e3c1af 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -3359,7 +3359,8 @@ migrate [--comp-xbzrle-cache] [--auto-converge] [auto-converge-initial] [auto-converge-increment] [--persistent-xml file] [--tls] [--postcopy-bandwidth bandwidth] - [--parallel [--parallel-connections connections]] + [--parallel [--parallel-connections connections] + [--comp-zlib-level][--comp-zstd-level]]

These should be moved two lines above after [--comp-xbzrle-cache]. And don't forget to add a space after ']'.

[--bandwidth bandwidth] [--tls-destination hostname] [--disks-uri URI] [--copy-storage-synchronous-writes]

@@ -3466,11 +3467,14 @@ option for this to work. 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. +can be tuned further. When used with --parallel, supported methods are "zlib" +and "zstd" and if no methods are specified, QEMU will deaults to "none". +Note that compression method is defferent from compress migration and multifd +migration. *--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.

The options for zlib and zstd compressions should be described here. I can imagine something along the following lines: 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. *--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. *--comp-zstd-level* sets the compression level when using "zstd" method. Values are in range from 0 to 20 and defaults to 1. Although the semantics of the values need to be described as well. And the same info needs to go to the first patch.

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 @@ -3486,7 +3490,11 @@ starting the migration. parallel connections. The number of such connections can be set using *--parallel-connections*. Parallel connections may help with saturating the network link between the source and the target and thus speeding up the -migration. +migration. *--comp-zlib-level* sets the compression level when using "zlib" +as multifd migration's compression mesthod. Values are in range from 0 to 9 +and defaults to 1. *--comp-zstd-level* sets the compression level when +using "zstd" as multifd migration's compression mesthod. Values are in range +from 0 to 20 and defaults to 1.

Running migration can be canceled by interrupting virsh (usually using ``Ctrl-C``) or by ``domjobabort`` command sent from another virsh instance. diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6b431bd1e5..bcf03e9567 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11095,6 +11095,14 @@ static const vshCmdOptDef opts_migrate[] = { .type = VSH_OT_INT, .help = N_("number of connections for parallel migration") }, + {.name = "comp-zlib-level", + .type = VSH_OT_INT, + .help = N_("zlib compression level used in parallel migration") + }, + {.name = "comp-zstd-level", + .type = VSH_OT_INT, + .help = N_("zstd compression level used in parallel migration") + },

Virsh options should always be added at the end, i.e., just before {.name = NULL}

{.name = "bandwidth", .type = VSH_OT_INT, .help = N_("migration bandwidth limit in MiB/s")

... Jirka