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

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

-----Original Message----- From: Jiri Denemark [mailto:jdenemar@redhat.com] Sent: Thursday, October 15, 2015 5:15 PM To: Feng, Shaohe Cc: libvir-list(a)redhat.com; Li, Liang Z; Qiao, Liyong Subject: Re: [PATCH V2 1/9] qemu_migration: Add support for mutil-thread compressed migration enable 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(a)intel.com&gt; > Signed-off-by: ShaoHe Feng <shaohe.feng(a)intel.com&gt; > --- > .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