On 2023/2/7 23:37, Jiri Denemark wrote:
> 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@xxxxxxxxxx>
>> ---
>> 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.
Thanks for your suggestion. I will try to merge this function into
'qemuMigrationParamsSetCompression ' in my next version.
I realize that zlib/zstd are compression method same with the others. I
should process them together and using flags to choose, not using flags
to choose the way I process them.
>
>> + 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.
Thanks for your suggestion, I wiil improve the logic here in my next
version.
Thanks, Jiang Jiacheng
>
>> + 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
>
