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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]