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

[Jiang Jiacheng]

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 at 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.

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
>