[libvirt] [PATCH V2 1/9] qemu_migration: Add support for mutil-thread compressed migration enable

[Jiri Denemark]

On Thu, Jul 09, 2015 at 21:01:49 +0800, ShaoHe Feng wrote:
> We need to set the mutil-thread compress capability as true to enable it.
> 
> Signed-off-by: Eli Qiao <liyong.qiao at intel.com>
> Signed-off-by: ShaoHe Feng <shaohe.feng at intel.com>
> ---
>  .gnulib                   |  2 +-
>  src/qemu/qemu_migration.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_migration.h |  9 +++++++-
>  src/qemu/qemu_monitor.c   |  2 +-
>  src/qemu/qemu_monitor.h   |  1 +
>  5 files changed, 67 insertions(+), 3 deletions(-)
> 
> diff --git a/.gnulib b/.gnulib
> index f39477d..875ec93 160000
> --- a/.gnulib
> +++ b/.gnulib
> @@ -1 +1 @@
> -Subproject commit f39477dba778e99392948dd3dd19ec0d46aee932
> +Subproject commit 875ec93e1501d2d2a8bab1b64fa66b8ceb51dc67

Drop this change to .gnulib

> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 7257182..891ddb6 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -2343,6 +2343,52 @@ qemuMigrationSetCompression(virQEMUDriverPtr driver,
>      return ret;
>  }
>  
> +int
> +qemuMigrationSetMultiThreadCompression(virQEMUDriverPtr driver,
> +                                       virDomainObjPtr vm,
> +                                       bool state,
> +                                       qemuDomainAsyncJob job)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    int ret = -1;
> +
> +    if (qemuDomainObjEnterMonitorAsync(driver, vm, job) < 0)
> +        return -1;
> +
> +    ret = qemuMonitorGetMigrationCapability(
> +                priv->mon,
> +                QEMU_MONITOR_MIGRATION_CAPS_MT_COMPRESS);

Some people don't like this formatting and prefer

       ret = qemuMonitorGetMigrationCapability(priv->mon,
                                               QEMU_MONITOR_MIGRATION_CAPS_MT_COMPRESS);

even though the result is longer than 80 characters. I don't mind either
way so it's up to you if you want to change it or not :-) [1]

> +
> +    if (ret < 0) {
> +        goto cleanup;
> +    } else if (ret == 0 && !state) {
> +        /* Unsupported but we want it off anyway */
> +        goto cleanup;
> +    } else if (ret == 0) {
> +        if (job == QEMU_ASYNC_JOB_MIGRATION_IN) {
> +            virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> +                           _("Multi-thread compressed migration is not supported by "
> +                             "target QEMU binary"));

Split the error message in two lines earlier so that both lines fit
within 80 columns.

> +        } else {
> +            virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> +                           _("Multi-thread compressed migration is not supported by "
> +                             "source QEMU binary"));

And here as well.

> +        }
> +        ret = -1;
> +        goto cleanup;
> +    }
> +
> +    ret = qemuMonitorSetMigrationCapability(
> +                priv->mon,
> +                QEMU_MONITOR_MIGRATION_CAPS_MT_COMPRESS,
> +                state);

[1] applies here too.

> +
> + cleanup:
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        ret = -1;
> +    return ret;
> +}
> +
>  static int
>  qemuMigrationSetAutoConverge(virQEMUDriverPtr driver,
>                               virDomainObjPtr vm,
> @@ -3374,6 +3420,11 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>                                      QEMU_ASYNC_JOB_MIGRATION_IN) < 0)
>          goto stop;
>  
> +    if (qemuMigrationSetMultiThreadCompression(driver, vm,
> +                                               flags & VIR_MIGRATE_MT_COMPRESSED,

This won't compile because the public flag is introduced later in patch
6/9. The introduction of this flag must either be moved here or to an
earlier standalone patch. Remember, "make all check syntax-check" should
pass after every single patch in a series.

However, since we already have VIR_MIGRATE_COMPRESSED flag and I can
imagine various other hypervisors could support their own compression
methods, I think using flags for selecting the compression method is
wrong. So what if we keep just VIR_MIGRATE_COMPRESSED flag and introduce
a new migration parameter to let the user select what compression method
they want to use (XBZRLE, multithreaded compression, ...) and each of
them could be further configurable with additional parameters. Each
hypervisor would also advertise a list of supported compression methods
via virConnectGetDomainCapabilities. QEMU would have XBZRLE method
selected by default for backward compatibility (it would have to be
advertised as the default method in virConnectGetDomainCapabilities
too).

So what about something like

/* compression method */
#define VIR_MIGRATE_PARAM_COMPRESSION "compression"

/* XBZRLE parameters */
#define VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE "compression.xbzrle.cache"

/* multithreaded compression parameters */
#define VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL "compression.mt.level"
#define VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS "compression.mt.threads"
#define VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS "compression.mt.dthreads"

Jirka