Thank Jirka for your comments. Will address them in the next version. > -----Original Message----- > From: Jiri Denemark [mailto:jdenemar@xxxxxxxxxx] > Sent: Thursday, October 15, 2015 5:15 PM > To: Feng, Shaohe > Cc: libvir-list@xxxxxxxxxx; 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@xxxxxxxxx> > > Signed-off-by: ShaoHe Feng <shaohe.feng@xxxxxxxxx> > > --- > > .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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list