[libvirt] [PATCH v4 0/5] migration: add multithread compression
Add means to turn multithread compression on during migration. Add means to pass compression parameters in migration command. Changes from v3 =============== 1. HMP support is dropped. 2. Switch to a different implementation of setting migration parameters. Version 3 sets all parameters when configuring migration compression. If parameter is not specified explicitly it is set to hardcoded default value. New version set only explicitly specified parameters instead. This has some reproducibility drawbacks which are nevertheless could be overcomed and this issue will be addressed in another series. 3. Unspecified values are presented by bitset fields rather then specific values of parameters itself. This is a bit more tedious implementation to use but if we take it we can pass all parameter values checking to QEMU. 4. Misc minor changes on Jiri comments. Eli Qiao (1): qemumonitorjsontest: add test for getting multithread compress params Nikolay Shirokovskiy (2): migration: add compress method option qemu: migration: add compression options ShaoHe Feng (2): qemu monitor: add multithread compress parameters accessors virsh: add compression options for migration include/libvirt/libvirt-domain.h | 44 +++++++ src/qemu/qemu_driver.c | 29 ++++- src/qemu/qemu_migration.c | 246 ++++++++++++++++++++++++++++++++++----- src/qemu/qemu_migration.h | 30 +++++ src/qemu/qemu_monitor.c | 24 +++- src/qemu/qemu_monitor.h | 18 +++ src/qemu/qemu_monitor_json.c | 110 +++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 + tests/qemumonitorjsontest.c | 53 +++++++++ tools/virsh-domain.c | 83 +++++++++++++ tools/virsh.pod | 25 +++- 11 files changed, 628 insertions(+), 39 deletions(-) -- 1.8.3.1
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- include/libvirt/libvirt-domain.h | 11 +++ src/qemu/qemu_driver.c | 29 +++++-- src/qemu/qemu_migration.c | 163 ++++++++++++++++++++++++++++++++------- src/qemu/qemu_migration.h | 23 ++++++ src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_monitor.h | 1 + 6 files changed, 195 insertions(+), 34 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 79c25df..b3a176f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -671,6 +671,8 @@ typedef enum { * when supported */ VIR_MIGRATE_UNSAFE = (1 << 9), /* force migration even if it is considered unsafe */ VIR_MIGRATE_OFFLINE = (1 << 10), /* offline migrate */ + /* Migration options could be specified further via VIR_MIGRATE_PARAM_COMPRESSION + * otherwise default options will be used */ VIR_MIGRATE_COMPRESSED = (1 << 11), /* compress data during migration */ VIR_MIGRATE_ABORT_ON_ERROR = (1 << 12), /* abort migration on I/O errors happened during migration */ VIR_MIGRATE_AUTO_CONVERGE = (1 << 13), /* force convergence */ @@ -773,6 +775,15 @@ typedef enum { */ # define VIR_MIGRATE_PARAM_MIGRATE_DISKS "migrate_disks" +/** + * VIR_MIGRATE_PARAM_COMPRESSION: + * + * virDomainMigrate* params multiple field: string list of compression methods + * that are used to compress migration traffic. + */ + +# define VIR_MIGRATE_PARAM_COMPRESSION "compression" + /* Domain migration. */ virDomainPtr virDomainMigrate (virDomainPtr domain, virConnectPtr dconn, unsigned long flags, const char *dname, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4bd4071..b27b8f0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12061,7 +12061,7 @@ qemuDomainMigratePrepare2(virConnectPtr dconn, ret = qemuMigrationPrepareDirect(driver, dconn, NULL, 0, NULL, NULL, /* No cookies */ uri_in, uri_out, - &def, origname, NULL, 0, NULL, flags); + &def, origname, NULL, 0, NULL, NULL, flags); cleanup: VIR_FREE(origname); @@ -12114,7 +12114,7 @@ qemuDomainMigratePerform(virDomainPtr dom, * Consume any cookie we were able to decode though */ ret = qemuMigrationPerform(driver, dom->conn, vm, - NULL, dconnuri, uri, NULL, NULL, 0, NULL, + NULL, dconnuri, uri, NULL, NULL, 0, NULL, NULL, cookie, cookielen, NULL, NULL, /* No output cookies in v2 */ flags, dname, resource, false); @@ -12287,7 +12287,7 @@ qemuDomainMigratePrepare3(virConnectPtr dconn, cookiein, cookieinlen, cookieout, cookieoutlen, uri_in, uri_out, - &def, origname, NULL, 0, NULL, flags); + &def, origname, NULL, 0, NULL, NULL, flags); cleanup: VIR_FREE(origname); @@ -12316,6 +12316,7 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, int nmigrate_disks; const char **migrate_disks = NULL; char *origname = NULL; + qemuMigrationCompressionPtr compression = NULL; int ret = -1; virCheckFlagsGoto(QEMU_MIGRATION_FLAGS, cleanup); @@ -12343,6 +12344,12 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, if (nmigrate_disks < 0) goto cleanup; + if (VIR_ALLOC(compression) < 0) + goto cleanup; + + if (qemuMigrationCompressionParseParams(compression, params, nparams) < 0) + goto cleanup; + if (flags & VIR_MIGRATE_TUNNELLED) { /* this is a logical error; we never should have gotten here with * VIR_MIGRATE_TUNNELLED set @@ -12364,9 +12371,11 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, cookieout, cookieoutlen, uri_in, uri_out, &def, origname, listenAddress, - nmigrate_disks, migrate_disks, flags); + nmigrate_disks, migrate_disks, + compression, flags); cleanup: + VIR_FREE(compression); VIR_FREE(migrate_disks); VIR_FREE(origname); virDomainDefFree(def); @@ -12498,7 +12507,7 @@ qemuDomainMigratePerform3(virDomainPtr dom, } return qemuMigrationPerform(driver, dom->conn, vm, xmlin, - dconnuri, uri, NULL, NULL, 0, NULL, + dconnuri, uri, NULL, NULL, 0, NULL, NULL, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource, true); @@ -12525,6 +12534,7 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, int nmigrate_disks; const char **migrate_disks = NULL; unsigned long long bandwidth = 0; + qemuMigrationCompressionPtr compression = NULL; int ret = -1; virCheckFlags(QEMU_MIGRATION_FLAGS, -1); @@ -12558,6 +12568,12 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, if (nmigrate_disks < 0) goto cleanup; + if (VIR_ALLOC(compression) < 0) + goto cleanup; + + if (qemuMigrationCompressionParseParams(compression, params, nparams) < 0) + goto cleanup; + if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -12568,10 +12584,11 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, ret = qemuMigrationPerform(driver, dom->conn, vm, dom_xml, dconnuri, uri, graphicsuri, listenAddress, - nmigrate_disks, migrate_disks, + nmigrate_disks, migrate_disks, compression, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, bandwidth, true); cleanup: + VIR_FREE(compression); VIR_FREE(migrate_disks); return ret; } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 64cbffa..5fcf132 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3299,6 +3299,39 @@ qemuMigrationPrepareIncoming(virDomainObjPtr vm, } static int +qemuMigrationSetCompression(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob job, + qemuMigrationCompressionPtr compression, + unsigned int flags) +{ + /* + * if compression methods are not set explicitly use flags to + * set default compression methods + */ + if (compression == NULL || compression->methods == 0) { + return qemuMigrationSetOption(driver, vm, + QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, + flags & VIR_MIGRATE_COMPRESSED, + job); + } + + if (qemuMigrationSetOption(driver, vm, + QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, + compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE, + job) < 0) + return -1; + + if (qemuMigrationSetOption(driver, vm, + QEMU_MONITOR_MIGRATION_CAPS_COMPRESS, + compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD, + job) < 0) + return -1; + + return 0; +} + +static int qemuMigrationPrepareAny(virQEMUDriverPtr driver, virConnectPtr dconn, const char *cookiein, @@ -3314,6 +3347,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, unsigned long flags) { virDomainObjPtr vm = NULL; @@ -3495,10 +3529,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, dataFD[1] = -1; /* 'st' owns the FD now & will close it */ } - if (qemuMigrationSetOption(driver, vm, - QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, - flags & VIR_MIGRATE_COMPRESSED, - QEMU_ASYNC_JOB_MIGRATION_IN) < 0) + if (qemuMigrationSetCompression(driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN, + compression, flags) < 0) goto stopjob; if (STREQ_NULLABLE(protocol, "rdma") && @@ -3637,7 +3669,7 @@ qemuMigrationPrepareTunnel(virQEMUDriverPtr driver, ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen, cookieout, cookieoutlen, def, origname, - st, NULL, 0, false, NULL, 0, NULL, flags); + st, NULL, 0, false, NULL, 0, NULL, NULL, flags); return ret; } @@ -3679,6 +3711,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, unsigned long flags) { unsigned short port = 0; @@ -3801,7 +3834,8 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, cookieout, cookieoutlen, def, origname, NULL, uri ? uri->scheme : "tcp", port, autoPort, listenAddress, - nmigrate_disks, migrate_disks, flags); + nmigrate_disks, migrate_disks, compression, + flags); cleanup: virURIFree(uri); VIR_FREE(hostname); @@ -4282,7 +4316,8 @@ qemuMigrationRun(virQEMUDriverPtr driver, virConnectPtr dconn, const char *graphicsuri, size_t nmigrate_disks, - const char **migrate_disks) + const char **migrate_disks, + qemuMigrationCompressionPtr compression) { int ret = -1; unsigned int migrate_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; @@ -4364,10 +4399,8 @@ qemuMigrationRun(virQEMUDriverPtr driver, goto cleanup; } - if (qemuMigrationSetOption(driver, vm, - QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, - flags & VIR_MIGRATE_COMPRESSED, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) + if (qemuMigrationSetCompression(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT, + compression, flags) < 0) goto cleanup; if (qemuMigrationSetOption(driver, vm, @@ -4581,7 +4614,8 @@ static int doNativeMigrate(virQEMUDriverPtr driver, virConnectPtr dconn, const char *graphicsuri, size_t nmigrate_disks, - const char **migrate_disks) + const char **migrate_disks, + qemuMigrationCompressionPtr compression) { qemuDomainObjPrivatePtr priv = vm->privateData; virURIPtr uribits = NULL; @@ -4631,7 +4665,8 @@ static int doNativeMigrate(virQEMUDriverPtr driver, ret = qemuMigrationRun(driver, vm, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, &spec, dconn, - graphicsuri, nmigrate_disks, migrate_disks); + graphicsuri, nmigrate_disks, migrate_disks, + compression); if (spec.destType == MIGRATION_DEST_FD) VIR_FORCE_CLOSE(spec.dest.fd.qemu); @@ -4655,7 +4690,8 @@ static int doTunnelMigrate(virQEMUDriverPtr driver, virConnectPtr dconn, const char *graphicsuri, size_t nmigrate_disks, - const char **migrate_disks) + const char **migrate_disks, + qemuMigrationCompressionPtr compression) { virNetSocketPtr sock = NULL; int ret = -1; @@ -4692,7 +4728,8 @@ static int doTunnelMigrate(virQEMUDriverPtr driver, ret = qemuMigrationRun(driver, vm, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, &spec, dconn, - graphicsuri, nmigrate_disks, migrate_disks); + graphicsuri, nmigrate_disks, migrate_disks, + compression); cleanup: if (spec.destType == MIGRATION_DEST_FD) { @@ -4803,12 +4840,12 @@ static int doPeer2PeerMigrate2(virQEMUDriverPtr driver, ret = doTunnelMigrate(driver, vm, st, NULL, 0, NULL, NULL, flags, resource, dconn, - NULL, 0, NULL); + NULL, 0, NULL, NULL); else ret = doNativeMigrate(driver, vm, uri_out, cookie, cookielen, NULL, NULL, /* No out cookie with v2 migration */ - flags, resource, dconn, NULL, 0, NULL); + flags, resource, dconn, NULL, 0, NULL, NULL); /* Perform failed. Make sure Finish doesn't overwrite the error */ if (ret < 0) @@ -4872,6 +4909,7 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, unsigned long long bandwidth, bool useParams, unsigned long flags) @@ -4949,6 +4987,10 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver, VIR_MIGRATE_PARAM_MIGRATE_DISKS, migrate_disks[i]) < 0) goto cleanup; + if (compression && + qemuMigrationCompressionDumpParams(compression, ¶ms, &nparams, + &maxparams) < 0) + goto cleanup; } if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) @@ -5034,13 +5076,13 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver, cookiein, cookieinlen, &cookieout, &cookieoutlen, flags, bandwidth, dconn, graphicsuri, - nmigrate_disks, migrate_disks); + nmigrate_disks, migrate_disks, compression); } else { ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, &cookieout, &cookieoutlen, flags, bandwidth, dconn, graphicsuri, - nmigrate_disks, migrate_disks); + nmigrate_disks, migrate_disks, compression); } /* Perform failed. Make sure Finish doesn't overwrite the error */ @@ -5215,6 +5257,7 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, unsigned long flags, const char *dname, unsigned long resource, @@ -5328,8 +5371,8 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver, if (*v3proto) { ret = doPeer2PeerMigrate3(driver, sconn, dconn, dconnuri, vm, xmlin, dname, uri, graphicsuri, listenAddress, - nmigrate_disks, migrate_disks, resource, - useParams, flags); + nmigrate_disks, migrate_disks, compression, + resource, useParams, flags); } else { ret = doPeer2PeerMigrate2(driver, sconn, dconn, vm, dconnuri, flags, dname, resource); @@ -5366,6 +5409,7 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, const char *cookiein, int cookieinlen, char **cookieout, @@ -5401,13 +5445,13 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver, if ((flags & (VIR_MIGRATE_TUNNELLED | VIR_MIGRATE_PEER2PEER))) { ret = doPeer2PeerMigrate(driver, conn, vm, xmlin, dconnuri, uri, graphicsuri, listenAddress, - nmigrate_disks, migrate_disks, + nmigrate_disks, migrate_disks, compression, flags, dname, resource, &v3proto); } else { qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM2); ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, cookieout, cookieoutlen, - flags, resource, NULL, NULL, 0, NULL); + flags, resource, NULL, NULL, 0, NULL, NULL); } if (ret < 0) goto endjob; @@ -5468,6 +5512,7 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver, const char *graphicsuri, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, const char *cookiein, int cookieinlen, char **cookieout, @@ -5493,7 +5538,7 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver, ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, NULL, graphicsuri, - nmigrate_disks, migrate_disks); + nmigrate_disks, migrate_disks, compression); if (ret < 0) { if (qemuMigrationRestoreDomainState(conn, vm)) { @@ -5535,6 +5580,7 @@ qemuMigrationPerform(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, const char *cookiein, int cookieinlen, char **cookieout, @@ -5564,7 +5610,7 @@ qemuMigrationPerform(virQEMUDriverPtr driver, return qemuMigrationPerformJob(driver, conn, vm, xmlin, dconnuri, uri, graphicsuri, listenAddress, nmigrate_disks, migrate_disks, - cookiein, cookieinlen, + compression, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource, v3proto); } else { @@ -5578,14 +5624,14 @@ qemuMigrationPerform(virQEMUDriverPtr driver, return qemuMigrationPerformPhase(driver, conn, vm, uri, graphicsuri, nmigrate_disks, migrate_disks, - cookiein, cookieinlen, + compression, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource); } else { return qemuMigrationPerformJob(driver, conn, vm, xmlin, NULL, uri, graphicsuri, listenAddress, nmigrate_disks, migrate_disks, - cookiein, cookieinlen, + compression, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource, v3proto); } @@ -6293,3 +6339,66 @@ qemuMigrationErrorReport(virQEMUDriverPtr driver, virSetError(err); virFreeError(err); } + +static int +qemuMigrationCompressMethodsAdd(qemuMigrationCompressMethods *methods, + qemuMigrationCompressMethods method) +{ + if (*methods & method) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Compression method is specified twice.")); + return -1; + } + + *methods |= method; + return 0; +} + +int +qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr params, int nparams) +{ + size_t i; + + for (i = 0; i < nparams; i++) { + if (STRNEQ(params[i].field, VIR_MIGRATE_PARAM_COMPRESSION)) + continue; + + if (STREQ(params[i].value.s, "xbzrle")) { + if (qemuMigrationCompressMethodsAdd(&compression->methods, + QEMU_MIGRATION_COMPESS_XBZRLE) < 0) + return -1; + } else if (STREQ(params[i].value.s, "multithread")) { + if (qemuMigrationCompressMethodsAdd(&compression->methods, + QEMU_MIGRATION_COMPESS_MULTITHREAD) < 0) + return -1; + } else { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Unsupported compression method")); + return -1; + } + } + + return 0; +} + +int +qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr *params, + int *nparams, + int *maxparams) +{ + if ((compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE) && + virTypedParamsAddString(params, nparams, maxparams, + VIR_MIGRATE_PARAM_COMPRESSION, + "xbzrle") < 0) + return -1; + + if ((compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD) && + virTypedParamsAddString(params, nparams, maxparams, + VIR_MIGRATE_PARAM_COMPRESSION, + "multithread") < 0) + return -1; + + return 0; +} diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 2c67a02..3cbe944 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -25,6 +25,9 @@ # include "qemu_conf.h" # include "qemu_domain.h" +typedef struct _qemuMigrationCompression qemuMigrationCompression; +typedef qemuMigrationCompression *qemuMigrationCompressionPtr; + /* All supported qemu migration flags. */ # define QEMU_MIGRATION_FLAGS \ (VIR_MIGRATE_LIVE | \ @@ -53,6 +56,8 @@ VIR_MIGRATE_PARAM_LISTEN_ADDRESS, VIR_TYPED_PARAM_STRING, \ VIR_MIGRATE_PARAM_MIGRATE_DISKS, VIR_TYPED_PARAM_STRING | \ VIR_TYPED_PARAM_MULTIPLE, \ + VIR_MIGRATE_PARAM_COMPRESSION, VIR_TYPED_PARAM_STRING | \ + VIR_TYPED_PARAM_MULTIPLE, \ NULL @@ -72,6 +77,22 @@ typedef enum { } qemuMigrationJobPhase; VIR_ENUM_DECL(qemuMigrationJobPhase) +typedef enum { + QEMU_MIGRATION_COMPESS_XBZRLE = (1 << 0), + QEMU_MIGRATION_COMPESS_MULTITHREAD = (1 << 1), +} qemuMigrationCompressMethods; + +struct _qemuMigrationCompression { + qemuMigrationCompressMethods methods; +}; + +int qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr params, int nparams); +int qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr *params, + int *nparams, + int *maxparams); + int qemuMigrationJobStart(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob job) @@ -134,6 +155,7 @@ int qemuMigrationPrepareDirect(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, unsigned long flags); int qemuMigrationPerform(virQEMUDriverPtr driver, @@ -146,6 +168,7 @@ int qemuMigrationPerform(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, const char *cookiein, int cookieinlen, char **cookieout, diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index ace3bb4..5e4461a 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -167,7 +167,7 @@ VIR_ENUM_IMPL(qemuMonitorMigrationStatus, VIR_ENUM_IMPL(qemuMonitorMigrationCaps, QEMU_MONITOR_MIGRATION_CAPS_LAST, - "xbzrle", "auto-converge", "rdma-pin-all", "events") + "xbzrle", "auto-converge", "rdma-pin-all", "events", "compress") VIR_ENUM_IMPL(qemuMonitorVMStatus, QEMU_MONITOR_VM_STATUS_LAST, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 4467a41..28620b5 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -531,6 +531,7 @@ typedef enum { QEMU_MONITOR_MIGRATION_CAPS_AUTO_CONVERGE, QEMU_MONITOR_MIGRATION_CAPS_RDMA_PIN_ALL, QEMU_MONITOR_MIGRATION_CAPS_EVENTS, + QEMU_MONITOR_MIGRATION_CAPS_COMPRESS, QEMU_MONITOR_MIGRATION_CAPS_LAST } qemuMonitorMigrationCaps; -- 1.8.3.1
On Fri, Mar 04, 2016 at 14:20:54 +0300, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- include/libvirt/libvirt-domain.h | 11 +++ src/qemu/qemu_driver.c | 29 +++++-- src/qemu/qemu_migration.c | 163 ++++++++++++++++++++++++++++++++------- src/qemu/qemu_migration.h | 23 ++++++ src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_monitor.h | 1 + 6 files changed, 195 insertions(+), 34 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 79c25df..b3a176f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -671,6 +671,8 @@ typedef enum { * when supported */ VIR_MIGRATE_UNSAFE = (1 << 9), /* force migration even if it is considered unsafe */ VIR_MIGRATE_OFFLINE = (1 << 10), /* offline migrate */ + /* Migration options could be specified further via VIR_MIGRATE_PARAM_COMPRESSION + * otherwise default options will be used */
Either put a short version of the above comment to the comment for VIR_MIGRATE_COMPRESSED or add such comment to migration APIs (or both), but the comment here is likely to confuse our documentation generator.
VIR_MIGRATE_COMPRESSED = (1 << 11), /* compress data during migration */ VIR_MIGRATE_ABORT_ON_ERROR = (1 << 12), /* abort migration on I/O errors happened during migration */ VIR_MIGRATE_AUTO_CONVERGE = (1 << 13), /* force convergence */ @@ -773,6 +775,15 @@ typedef enum { */ # define VIR_MIGRATE_PARAM_MIGRATE_DISKS "migrate_disks"
+/** + * VIR_MIGRATE_PARAM_COMPRESSION: + * + * virDomainMigrate* params multiple field: string list of compression methods + * that are used to compress migration traffic. + */
What is a "string list"? Is it a single string with method names separated by something, an array of strings, or something completely different? Oh, looking at the rest of this patch, it's the case of allowing multiple instances of the same parameter. The documentation should say that, e.g., "name of the method used to compress migration traffic. The parameter may be specified multiple times if more than one method should be used".
+
Remove the empty line here.
+# define VIR_MIGRATE_PARAM_COMPRESSION "compression" + /* Domain migration. */ virDomainPtr virDomainMigrate (virDomainPtr domain, virConnectPtr dconn, unsigned long flags, const char *dname, ... diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 64cbffa..5fcf132 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3299,6 +3299,39 @@ qemuMigrationPrepareIncoming(virDomainObjPtr vm, }
static int +qemuMigrationSetCompression(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob job, + qemuMigrationCompressionPtr compression, + unsigned int flags) +{ + /* + * if compression methods are not set explicitly use flags to + * set default compression methods + */ + if (compression == NULL || compression->methods == 0) { + return qemuMigrationSetOption(driver, vm, + QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, + flags & VIR_MIGRATE_COMPRESSED, + job); + }
I think it would be cleaner if qemuMigrationCompressionParseParams got the flags and set QEMU_MIGRATION_COMPESS_XBZRLE in compression->methods in case no compression parametr is used. Doing that would even fix a bug in your code, because even if no VIR_MIGRATE_PARAM_COMPRESSION parameter is used, you still want to explicitly disable any supported compression capability. In other words, you'd still have to call qemuMigrationSetOption for both XBZRLE and MULTITHREAD in the if statement above. Changing qemuMigrationCompressionParseParams to do the right thing will avoid this code duplication.
+ + if (qemuMigrationSetOption(driver, vm, + QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, + compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE, + job) < 0) + return -1; + + if (qemuMigrationSetOption(driver, vm, + QEMU_MONITOR_MIGRATION_CAPS_COMPRESS, + compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD, + job) < 0) + return -1; + + return 0; +} + +static int qemuMigrationPrepareAny(virQEMUDriverPtr driver, virConnectPtr dconn, const char *cookiein, ... @@ -6293,3 +6339,66 @@ qemuMigrationErrorReport(virQEMUDriverPtr driver, virSetError(err); virFreeError(err); } + +static int +qemuMigrationCompressMethodsAdd(qemuMigrationCompressMethods *methods, + qemuMigrationCompressMethods method) +{ + if (*methods & method) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Compression method is specified twice.")); + return -1; + } + + *methods |= method; + return 0; +}
I don't think there is a reason to split the above trivial code into a separate function. The main reason is that the following function should be simplified.
+ +int +qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr params, int nparams) +{ + size_t i; + + for (i = 0; i < nparams; i++) { + if (STRNEQ(params[i].field, VIR_MIGRATE_PARAM_COMPRESSION)) + continue;
I think using virTypedParamsGetStringList to get all VIR_MIGRATE_PARAM_COMPRESSION parameters and then walking through them in a loop would look a bit nicer.
+ + if (STREQ(params[i].value.s, "xbzrle")) { + if (qemuMigrationCompressMethodsAdd(&compression->methods, + QEMU_MIGRATION_COMPESS_XBZRLE) < 0) + return -1; + } else if (STREQ(params[i].value.s, "multithread")) { + if (qemuMigrationCompressMethodsAdd(&compression->methods, + QEMU_MIGRATION_COMPESS_MULTITHREAD) < 0) + return -1; + } else { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Unsupported compression method")); + return -1; + }
Anyway, this is very ugly. You should change qemuMigrationCompressMethods to be usable with our VIR_ENUM_{IMPL,DECL} macros and use them here and in the *DumpParams function.
+ }
And set QEMU_MIGRATION_COMPESS_XBZRLE method if no param is passed and flags enabled compression.
+ + return 0; +} + +int +qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr *params, + int *nparams, + int *maxparams) +{ + if ((compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE) && + virTypedParamsAddString(params, nparams, maxparams, + VIR_MIGRATE_PARAM_COMPRESSION, + "xbzrle") < 0) + return -1; + + if ((compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD) && + virTypedParamsAddString(params, nparams, maxparams, + VIR_MIGRATE_PARAM_COMPRESSION, + "multithread") < 0) + return -1;
For backward compatibility this would just need to keep params empty if xbzrle was the only compression method specified and use only flags to express that.
+ + return 0; +} diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 2c67a02..3cbe944 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -25,6 +25,9 @@ # include "qemu_conf.h" # include "qemu_domain.h"
+typedef struct _qemuMigrationCompression qemuMigrationCompression; +typedef qemuMigrationCompression *qemuMigrationCompressionPtr; + /* All supported qemu migration flags. */ # define QEMU_MIGRATION_FLAGS \ (VIR_MIGRATE_LIVE | \ @@ -53,6 +56,8 @@ VIR_MIGRATE_PARAM_LISTEN_ADDRESS, VIR_TYPED_PARAM_STRING, \ VIR_MIGRATE_PARAM_MIGRATE_DISKS, VIR_TYPED_PARAM_STRING | \ VIR_TYPED_PARAM_MULTIPLE, \ + VIR_MIGRATE_PARAM_COMPRESSION, VIR_TYPED_PARAM_STRING | \ + VIR_TYPED_PARAM_MULTIPLE, \ NULL
@@ -72,6 +77,22 @@ typedef enum { } qemuMigrationJobPhase; VIR_ENUM_DECL(qemuMigrationJobPhase)
+typedef enum { + QEMU_MIGRATION_COMPESS_XBZRLE = (1 << 0), + QEMU_MIGRATION_COMPESS_MULTITHREAD = (1 << 1), +} qemuMigrationCompressMethods; + +struct _qemuMigrationCompression { + qemuMigrationCompressMethods methods;
This should rather be unsigned long (or unsigned long long).
+}; + +int qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr params, int nparams); +int qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr *params, + int *nparams, + int *maxparams); + int qemuMigrationJobStart(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob job) ...
Jirka
On 18.03.2016 16:47, Jiri Denemark wrote:
On Fri, Mar 04, 2016 at 14:20:54 +0300, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- include/libvirt/libvirt-domain.h | 11 +++ src/qemu/qemu_driver.c | 29 +++++-- src/qemu/qemu_migration.c | 163 ++++++++++++++++++++++++++++++++------- src/qemu/qemu_migration.h | 23 ++++++ src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_monitor.h | 1 + 6 files changed, 195 insertions(+), 34 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 79c25df..b3a176f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -671,6 +671,8 @@ typedef enum { * when supported */ VIR_MIGRATE_UNSAFE = (1 << 9), /* force migration even if it is considered unsafe */ VIR_MIGRATE_OFFLINE = (1 << 10), /* offline migrate */ + /* Migration options could be specified further via VIR_MIGRATE_PARAM_COMPRESSION + * otherwise default options will be used */
Either put a short version of the above comment to the comment for VIR_MIGRATE_COMPRESSED or add such comment to migration APIs (or both), but the comment here is likely to confuse our documentation generator.
VIR_MIGRATE_COMPRESSED = (1 << 11), /* compress data during migration */ VIR_MIGRATE_ABORT_ON_ERROR = (1 << 12), /* abort migration on I/O errors happened during migration */ VIR_MIGRATE_AUTO_CONVERGE = (1 << 13), /* force convergence */ @@ -773,6 +775,15 @@ typedef enum { */ # define VIR_MIGRATE_PARAM_MIGRATE_DISKS "migrate_disks"
+/** + * VIR_MIGRATE_PARAM_COMPRESSION: + * + * virDomainMigrate* params multiple field: string list of compression methods + * that are used to compress migration traffic. + */
What is a "string list"? Is it a single string with method names separated by something, an array of strings, or something completely different? Oh, looking at the rest of this patch, it's the case of allowing multiple instances of the same parameter. The documentation should say that, e.g., "name of the method used to compress migration traffic. The parameter may be specified multiple times if more than one method should be used".
+
Remove the empty line here.
+# define VIR_MIGRATE_PARAM_COMPRESSION "compression" + /* Domain migration. */ virDomainPtr virDomainMigrate (virDomainPtr domain, virConnectPtr dconn, unsigned long flags, const char *dname, ... diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 64cbffa..5fcf132 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3299,6 +3299,39 @@ qemuMigrationPrepareIncoming(virDomainObjPtr vm, }
static int +qemuMigrationSetCompression(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob job, + qemuMigrationCompressionPtr compression, + unsigned int flags) +{ + /* + * if compression methods are not set explicitly use flags to + * set default compression methods + */ + if (compression == NULL || compression->methods == 0) { + return qemuMigrationSetOption(driver, vm, + QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, + flags & VIR_MIGRATE_COMPRESSED, + job); + }
I think it would be cleaner if qemuMigrationCompressionParseParams got the flags and set QEMU_MIGRATION_COMPESS_XBZRLE in compression->methods in case no compression parametr is used. Doing that would even fix a bug in your code, because even if no VIR_MIGRATE_PARAM_COMPRESSION parameter is used, you still want to explicitly disable any supported compression capability. In other words, you'd still have to call qemuMigrationSetOption for both XBZRLE and MULTITHREAD in the if statement above. Changing qemuMigrationCompressionParseParams to do the right thing will avoid this code duplication.
Hi, Jiri. Thanx for reviewing. I think we'd better stick with this approach. The reason is that we can get here thru old API where only flags are set and compression parameter is set to NULL on the way. As to explicitly setting state of all compression methods we can resolve flag into qemuMigrationCompressMethods and use same code as in case of custom compression.
+ + if (qemuMigrationSetOption(driver, vm, + QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, + compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE, + job) < 0) + return -1; + + if (qemuMigrationSetOption(driver, vm, + QEMU_MONITOR_MIGRATION_CAPS_COMPRESS, + compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD, + job) < 0) + return -1; + + return 0; +} + +static int qemuMigrationPrepareAny(virQEMUDriverPtr driver, virConnectPtr dconn, const char *cookiein, ... @@ -6293,3 +6339,66 @@ qemuMigrationErrorReport(virQEMUDriverPtr driver, virSetError(err); virFreeError(err); } + +static int +qemuMigrationCompressMethodsAdd(qemuMigrationCompressMethods *methods, + qemuMigrationCompressMethods method) +{ + if (*methods & method) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Compression method is specified twice.")); + return -1; + } + + *methods |= method; + return 0; +}
I don't think there is a reason to split the above trivial code into a separate function. The main reason is that the following function should be simplified.
+ +int +qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr params, int nparams) +{ + size_t i; + + for (i = 0; i < nparams; i++) { + if (STRNEQ(params[i].field, VIR_MIGRATE_PARAM_COMPRESSION)) + continue;
I think using virTypedParamsGetStringList to get all VIR_MIGRATE_PARAM_COMPRESSION parameters and then walking through them in a loop would look a bit nicer.
Well, this way we can remove the the check above at the price of introducing variables and memory managment. Also we are not going to use the extracted list outside of the function. Is it worth the effort?
+ + if (STREQ(params[i].value.s, "xbzrle")) { + if (qemuMigrationCompressMethodsAdd(&compression->methods, + QEMU_MIGRATION_COMPESS_XBZRLE) < 0) + return -1; + } else if (STREQ(params[i].value.s, "multithread")) { + if (qemuMigrationCompressMethodsAdd(&compression->methods, + QEMU_MIGRATION_COMPESS_MULTITHREAD) < 0) + return -1; + } else { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Unsupported compression method")); + return -1; + }
Anyway, this is very ugly. You should change qemuMigrationCompressMethods to be usable with our VIR_ENUM_{IMPL,DECL} macros and use them here and in the *DumpParams function.
AFAIU I can not use VIR_ENUM_* family because I have flags and not sequential values in enum. I can introduce another family and set of related functions to deal with flags. Some generics to parse flags to virTypedParameterPtr back and forth. Do you think it is time to take this way?
+ }
And set QEMU_MIGRATION_COMPESS_XBZRLE method if no param is passed and flags enabled compression.
+ + return 0; +} + +int +qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr *params, + int *nparams, + int *maxparams) +{ + if ((compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE) && + virTypedParamsAddString(params, nparams, maxparams, + VIR_MIGRATE_PARAM_COMPRESSION, + "xbzrle") < 0) + return -1; + + if ((compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD) && + virTypedParamsAddString(params, nparams, maxparams, + VIR_MIGRATE_PARAM_COMPRESSION, + "multithread") < 0) + return -1;
For backward compatibility this would just need to keep params empty if xbzrle was the only compression method specified and use only flags to express that.
I suggest that we don't mix flags and compression parameters here and in parse function too. This way we do not get backward compatibility issues. If VIR_MIGRATE_COMPRESSED is specified we don't touch it and do not convert to compression parameters. This way the old value space is passed transparently by the old means. On dump compression.methods will be 0 and we do not add params unrecognized by older libvirt. If we start to mix compression in flags and parameters we have check on dump that new value space is representable in old value space and probably have to keep this place up to date with compression methods and their tunables. The only drawback I see on this path is that we have somewhat obscure check 'compression == NULL || compression->methods == 0) in qemuMigrationSetCompression(why == 0 check for?). The first check is for the non params API calls where compression is just not setted. The second is for params API but when compression is not specified by newly introduced means and we should fallback to handling flag value.
+ + return 0; +} diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 2c67a02..3cbe944 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -25,6 +25,9 @@ # include "qemu_conf.h" # include "qemu_domain.h"
+typedef struct _qemuMigrationCompression qemuMigrationCompression; +typedef qemuMigrationCompression *qemuMigrationCompressionPtr; + /* All supported qemu migration flags. */ # define QEMU_MIGRATION_FLAGS \ (VIR_MIGRATE_LIVE | \ @@ -53,6 +56,8 @@ VIR_MIGRATE_PARAM_LISTEN_ADDRESS, VIR_TYPED_PARAM_STRING, \ VIR_MIGRATE_PARAM_MIGRATE_DISKS, VIR_TYPED_PARAM_STRING | \ VIR_TYPED_PARAM_MULTIPLE, \ + VIR_MIGRATE_PARAM_COMPRESSION, VIR_TYPED_PARAM_STRING | \ + VIR_TYPED_PARAM_MULTIPLE, \ NULL
@@ -72,6 +77,22 @@ typedef enum { } qemuMigrationJobPhase; VIR_ENUM_DECL(qemuMigrationJobPhase)
+typedef enum { + QEMU_MIGRATION_COMPESS_XBZRLE = (1 << 0), + QEMU_MIGRATION_COMPESS_MULTITHREAD = (1 << 1), +} qemuMigrationCompressMethods; + +struct _qemuMigrationCompression { + qemuMigrationCompressMethods methods;
This should rather be unsigned long (or unsigned long long).
+}; + +int qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr params, int nparams); +int qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr *params, + int *nparams, + int *maxparams); + int qemuMigrationJobStart(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob job) ...
Jirka
On Wed, Mar 23, 2016 at 13:22:45 +0300, Nikolay Shirokovskiy wrote:
On 18.03.2016 16:47, Jiri Denemark wrote:
static int +qemuMigrationSetCompression(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob job, + qemuMigrationCompressionPtr compression, + unsigned int flags) +{ + /* + * if compression methods are not set explicitly use flags to + * set default compression methods + */ + if (compression == NULL || compression->methods == 0) { + return qemuMigrationSetOption(driver, vm, + QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, + flags & VIR_MIGRATE_COMPRESSED, + job); + }
I think it would be cleaner if qemuMigrationCompressionParseParams got the flags and set QEMU_MIGRATION_COMPESS_XBZRLE in compression->methods in case no compression parametr is used. Doing that would even fix a bug in your code, because even if no VIR_MIGRATE_PARAM_COMPRESSION parameter is used, you still want to explicitly disable any supported compression capability. In other words, you'd still have to call qemuMigrationSetOption for both XBZRLE and MULTITHREAD in the if statement above. Changing qemuMigrationCompressionParseParams to do the right thing will avoid this code duplication.
Hi, Jiri. Thanx for reviewing.
I think we'd better stick with this approach.
It's quite unclear which approach you refer to by "this" :-)
The reason is that we can get here thru old API where only flags are set and compression parameter is set to NULL on the way.
Yes, and if we always do the right thing in qemuMigrationCompressionParseParams we don't need to think about backward compatibility elsewhere and complicate the code there.
+ + if (qemuMigrationSetOption(driver, vm, + QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, + compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE, + job) < 0) + return -1; + + if (qemuMigrationSetOption(driver, vm, + QEMU_MONITOR_MIGRATION_CAPS_COMPRESS, + compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD, + job) < 0) + return -1; + + return 0; +} + +static int qemuMigrationPrepareAny(virQEMUDriverPtr driver, virConnectPtr dconn, const char *cookiein, ... @@ -6293,3 +6339,66 @@ qemuMigrationErrorReport(virQEMUDriverPtr driver, virSetError(err); virFreeError(err); } + +static int +qemuMigrationCompressMethodsAdd(qemuMigrationCompressMethods *methods, + qemuMigrationCompressMethods method) +{ + if (*methods & method) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Compression method is specified twice.")); + return -1; + } + + *methods |= method; + return 0; +}
I don't think there is a reason to split the above trivial code into a separate function. The main reason is that the following function should be simplified.
+ +int +qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr params, int nparams) +{ + size_t i; + + for (i = 0; i < nparams; i++) { + if (STRNEQ(params[i].field, VIR_MIGRATE_PARAM_COMPRESSION)) + continue;
I think using virTypedParamsGetStringList to get all VIR_MIGRATE_PARAM_COMPRESSION parameters and then walking through them in a loop would look a bit nicer.
Well, this way we can remove the the check above at the price of introducing variables and memory managment. Also we are not going to use the extracted list outside of the function. Is it worth the effort?
The code would be a bit cleaner that way IMHO, but either way works.
+ + if (STREQ(params[i].value.s, "xbzrle")) { + if (qemuMigrationCompressMethodsAdd(&compression->methods, + QEMU_MIGRATION_COMPESS_XBZRLE) < 0) + return -1; + } else if (STREQ(params[i].value.s, "multithread")) { + if (qemuMigrationCompressMethodsAdd(&compression->methods, + QEMU_MIGRATION_COMPESS_MULTITHREAD) < 0) + return -1; + } else { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Unsupported compression method")); + return -1; + }
Anyway, this is very ugly. You should change qemuMigrationCompressMethods to be usable with our VIR_ENUM_{IMPL,DECL} macros and use them here and in the *DumpParams function.
AFAIU I can not use VIR_ENUM_* family because I have flags and not sequential values in enum. I can introduce another family and set of related functions to deal with flags. Some generics to parse flags to virTypedParameterPtr back and forth. Do you think it is time to take this way?
Well, the enum should contain sequential numbers so that you can use VIR_DENUM_* for it. The code enabling them can easily do 1 << method to set the appropriate bit.
+ }
And set QEMU_MIGRATION_COMPESS_XBZRLE method if no param is passed and flags enabled compression.
+ + return 0; +} + +int +qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr *params, + int *nparams, + int *maxparams) +{ + if ((compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE) && + virTypedParamsAddString(params, nparams, maxparams, + VIR_MIGRATE_PARAM_COMPRESSION, + "xbzrle") < 0) + return -1; + + if ((compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD) && + virTypedParamsAddString(params, nparams, maxparams, + VIR_MIGRATE_PARAM_COMPRESSION, + "multithread") < 0) + return -1;
For backward compatibility this would just need to keep params empty if xbzrle was the only compression method specified and use only flags to express that.
I suggest that we don't mix flags and compression parameters here and in parse function too. This way we do not get backward compatibility issues. If VIR_MIGRATE_COMPRESSED is specified we don't touch it and do not convert to compression parameters. This way the old value space is passed transparently by the old means. On dump compression.methods will be 0 and we do not add params unrecognized by older libvirt.
The backward compatibility code will be limited to Parse/Dump parameters and the rest of the code will just see a nice struct of compression parameters without having to worry about how it was called. And dealing with it in *Dump is easy, just set don't output any parameters if only XBZRLE method was selected and no compression parameter is set. Jirka
On 23.03.2016 18:57, Jiri Denemark wrote:
On Wed, Mar 23, 2016 at 13:22:45 +0300, Nikolay Shirokovskiy wrote:
On 18.03.2016 16:47, Jiri Denemark wrote:
static int +qemuMigrationSetCompression(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob job, + qemuMigrationCompressionPtr compression, + unsigned int flags) +{ + /* + * if compression methods are not set explicitly use flags to + * set default compression methods + */ + if (compression == NULL || compression->methods == 0) { + return qemuMigrationSetOption(driver, vm, + QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, + flags & VIR_MIGRATE_COMPRESSED, + job); + }
I think it would be cleaner if qemuMigrationCompressionParseParams got the flags and set QEMU_MIGRATION_COMPESS_XBZRLE in compression->methods in case no compression parametr is used. Doing that would even fix a bug in your code, because even if no VIR_MIGRATE_PARAM_COMPRESSION parameter is used, you still want to explicitly disable any supported compression capability. In other words, you'd still have to call qemuMigrationSetOption for both XBZRLE and MULTITHREAD in the if statement above. Changing qemuMigrationCompressionParseParams to do the right thing will avoid this code duplication.
Hi, Jiri. Thanx for reviewing.
I think we'd better stick with this approach.
It's quite unclear which approach you refer to by "this" :-)
The reason is that we can get here thru old API where only flags are set and compression parameter is set to NULL on the way.
Yes, and if we always do the right thing in qemuMigrationCompressionParseParams we don't need to think about backward compatibility elsewhere and complicate the code there.
...
+ }
And set QEMU_MIGRATION_COMPESS_XBZRLE method if no param is passed and flags enabled compression.
+ + return 0; +} + +int +qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression, + virTypedParameterPtr *params, + int *nparams, + int *maxparams) +{ + if ((compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE) && + virTypedParamsAddString(params, nparams, maxparams, + VIR_MIGRATE_PARAM_COMPRESSION, + "xbzrle") < 0) + return -1; + + if ((compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD) && + virTypedParamsAddString(params, nparams, maxparams, + VIR_MIGRATE_PARAM_COMPRESSION, + "multithread") < 0) + return -1;
For backward compatibility this would just need to keep params empty if xbzrle was the only compression method specified and use only flags to express that.
I suggest that we don't mix flags and compression parameters here and in parse function too. This way we do not get backward compatibility issues. If VIR_MIGRATE_COMPRESSED is specified we don't touch it and do not convert to compression parameters. This way the old value space is passed transparently by the old means. On dump compression.methods will be 0 and we do not add params unrecognized by older libvirt.
The backward compatibility code will be limited to Parse/Dump parameters and the rest of the code will just see a nice struct of compression parameters without having to worry about how it was called. And dealing with it in *Dump is easy, just set don't output any parameters if only XBZRLE method was selected and no compression parameter is set.
Jirka
Then I should never pass NULL value of compression pointer anywhere. That is I should fix all the places where this is done in current version. Below is the complete list of functions where NULL is passed: qemuDomainMigratePrepare2 qemuDomainMigratePrepare3 qemuDomainMigratePerform qemuDomainMigratePerform3 qemuMigrationPrepareTunnel doPeer2PeerMigrate2 qemuMigrationPerformJob I need to add code to parse/free in every of this places. This is my concern. Except this I totally agree that trying to put backward compatibility issues in one place like parse/dump functions is good. But on this way I still need to touch a lot of places in less trivial way (in comparsion to don't mixing flags / compression parameters). Nikolay
On Thu, Mar 24, 2016 at 11:08:31 +0300, Nikolay Shirokovskiy wrote:
The backward compatibility code will be limited to Parse/Dump parameters and the rest of the code will just see a nice struct of compression parameters without having to worry about how it was called. And dealing with it in *Dump is easy, just set don't output any parameters if only XBZRLE method was selected and no compression parameter is set.
Then I should never pass NULL value of compression pointer anywhere. That is I should fix all the places where this is done in current version. Below is the complete list of functions where NULL is passed:
qemuDomainMigratePrepare2 qemuDomainMigratePrepare3 qemuDomainMigratePerform qemuDomainMigratePerform3
None of these accepts any compression parameters (except for the flag itself) so you could use a simple struct with only xbzrle set (or not depending on the flag) and place it on the stack, but the code would look cleaner if you just called the parse/free function too even though they wouldn't do much without any typed parameters passed in.
qemuMigrationPrepareTunnel doPeer2PeerMigrate2 qemuMigrationPerformJob
I need to add code to parse/free in every of this places. This is my concern. Except this I totally agree that trying to put backward compatibility issues in one place like parse/dump functions is good. But on this way I still need to touch a lot of places in less trivial way (in comparsion to don't mixing flags / compression parameters).
Right, but you touch various entry points and the internal code that actually does something interesting doesn't need to care about the origin of compression parameters. That said, I didn't really try changing the code to see how it looks like but having a single structure describing compression methods and parameters no matter how they were specified by a user seems cleaner to me. Jirka
On 31.03.2016 09:53, Jiri Denemark wrote:
On Thu, Mar 24, 2016 at 11:08:31 +0300, Nikolay Shirokovskiy wrote:
The backward compatibility code will be limited to Parse/Dump parameters and the rest of the code will just see a nice struct of compression parameters without having to worry about how it was called. And dealing with it in *Dump is easy, just set don't output any parameters if only XBZRLE method was selected and no compression parameter is set.
Then I should never pass NULL value of compression pointer anywhere. That is I should fix all the places where this is done in current version. Below is the complete list of functions where NULL is passed:
qemuDomainMigratePrepare2 qemuDomainMigratePrepare3 qemuDomainMigratePerform qemuDomainMigratePerform3
None of these accepts any compression parameters (except for the flag itself) so you could use a simple struct with only xbzrle set (or not depending on the flag) and place it on the stack, but the code would look cleaner if you just called the parse/free function too even though they wouldn't do much without any typed parameters passed in.
qemuMigrationPrepareTunnel doPeer2PeerMigrate2 qemuMigrationPerformJob
I need to add code to parse/free in every of this places. This is my concern. Except this I totally agree that trying to put backward compatibility issues in one place like parse/dump functions is good. But on this way I still need to touch a lot of places in less trivial way (in comparsion to don't mixing flags / compression parameters).
Right, but you touch various entry points and the internal code that actually does something interesting doesn't need to care about the origin of compression parameters. That said, I didn't really try changing the code to see how it looks like but having a single structure describing compression methods and parameters no matter how they were specified by a user seems cleaner to me.
Jirka
Thanx for clarification! Nikolay
From: ShaoHe Feng <shaohe.feng@intel.com> Signed-off-by: ShaoHe Feng <shaohe.feng@intel.com> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_monitor.c | 22 +++++++++ src/qemu/qemu_monitor.h | 17 +++++++ src/qemu/qemu_monitor_json.c | 110 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++ 4 files changed, 154 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5e4461a..21c1df6 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2157,6 +2157,28 @@ qemuMonitorSetMigrationCacheSize(qemuMonitorPtr mon, int +qemuMonitorGetMigrationParameters(qemuMonitorPtr mon, + qemuMonitorMigrationParametersPtr params) +{ + QEMU_CHECK_MONITOR_JSON(mon); + + return qemuMonitorJSONGetMigrationParameters(mon, params); +} + +int +qemuMonitorSetMigrationParameters(qemuMonitorPtr mon, + qemuMonitorMigrationParametersPtr params) +{ + VIR_DEBUG("level=%d threads=%d dthreads=%d", + params->level, params->threads, params->dthreads); + + QEMU_CHECK_MONITOR_JSON(mon); + + return qemuMonitorJSONSetMigrationParameters(mon, params); +} + + +int qemuMonitorGetMigrationStats(qemuMonitorPtr mon, qemuMonitorMigrationStatsPtr stats) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 28620b5..b28b431 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -469,6 +469,23 @@ int qemuMonitorGetMigrationCacheSize(qemuMonitorPtr mon, int qemuMonitorSetMigrationCacheSize(qemuMonitorPtr mon, unsigned long long cacheSize); +typedef struct _qemuMonitorMigrationParameters qemuMonitorMigrationParameters; +typedef qemuMonitorMigrationParameters *qemuMonitorMigrationParametersPtr; +struct _qemuMonitorMigrationParameters { + unsigned int level_set : 1; + unsigned int threads_set : 1; + unsigned int dthreads_set : 1; + + int level; + int threads; + int dthreads; +}; + +int qemuMonitorGetMigrationParameters(qemuMonitorPtr mon, + qemuMonitorMigrationParametersPtr params); +int qemuMonitorSetMigrationParameters(qemuMonitorPtr mon, + qemuMonitorMigrationParametersPtr params); + typedef enum { QEMU_MONITOR_MIGRATION_STATUS_INACTIVE, QEMU_MONITOR_MIGRATION_STATUS_SETUP, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8352e53..999c644 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2492,6 +2492,116 @@ qemuMonitorJSONSetMigrationCacheSize(qemuMonitorPtr mon, } +int +qemuMonitorJSONGetMigrationParameters(qemuMonitorPtr mon, + qemuMonitorMigrationParametersPtr params) +{ + int ret = -1; + virJSONValuePtr result; + virJSONValuePtr cmd = NULL; + virJSONValuePtr reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-migrate-parameters", NULL))) + return -1; + + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) + goto cleanup; + + if ((ret = qemuMonitorJSONCheckError(cmd, reply)) < 0) + goto cleanup; + + if (!(result = virJSONValueObjectGet(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-migrate-parameters reply was missing " + "'return' data")); + goto cleanup; + } + + if (virJSONValueObjectGetNumberInt(result, "compress-level", + ¶ms->level) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed/missing compress-level " + "in migrate parameters")); + goto cleanup; + } + params->level_set = 1; + + if (virJSONValueObjectGetNumberInt(result, "compress-threads", + ¶ms->threads) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed/missing compress-threads " + "in migrate parameters")); + goto cleanup; + } + params->threads_set = 1; + + if (virJSONValueObjectGetNumberInt(result, "decompress-threads", + ¶ms->dthreads) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed/missing decompress-threads " + "in migrate parameters")); + goto cleanup; + } + params->dthreads_set = 1; + + ret = 0; + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + +int +qemuMonitorJSONSetMigrationParameters(qemuMonitorPtr mon, + qemuMonitorMigrationParametersPtr params) +{ + int ret = -1; + virJSONValuePtr cmd = NULL; + virJSONValuePtr args = NULL; + virJSONValuePtr reply = NULL; + + if (!(cmd = virJSONValueNewObject())) + goto cleanup; + + if (virJSONValueObjectAppendString(cmd, "execute", + "migrate-set-parameters") < 0) + goto cleanup; + + if (!(args = virJSONValueNewObject())) + goto cleanup; + + if (params->level_set && + virJSONValueObjectAppendNumberInt(args, "compress-level", + params->level) < 0) + goto cleanup; + + if (params->threads_set && + virJSONValueObjectAppendNumberInt(args, "compress-threads", + params->threads) < 0) + goto cleanup; + + if (params->dthreads_set && + virJSONValueObjectAppendNumberInt(args, "compress-dthreads", + params->dthreads) < 0) + goto cleanup; + + if (virJSONValueObjectAppend(cmd, "arguments", args) < 0) + goto cleanup; + args = NULL; + + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) + goto cleanup; + + ret = qemuMonitorJSONCheckError(cmd, reply); + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(args); + virJSONValueFree(reply); + return ret; +} + + static int qemuMonitorJSONGetMigrationStatsReply(virJSONValuePtr reply, qemuMonitorMigrationStatsPtr stats) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 4068187..115882c 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -126,6 +126,11 @@ int qemuMonitorJSONGetMigrationCacheSize(qemuMonitorPtr mon, int qemuMonitorJSONSetMigrationCacheSize(qemuMonitorPtr mon, unsigned long long cacheSize); +int qemuMonitorJSONGetMigrationParameters(qemuMonitorPtr mon, + qemuMonitorMigrationParametersPtr params); +int qemuMonitorJSONSetMigrationParameters(qemuMonitorPtr mon, + qemuMonitorMigrationParametersPtr params); + int qemuMonitorJSONGetMigrationStats(qemuMonitorPtr mon, qemuMonitorMigrationStatsPtr stats); -- 1.8.3.1
On Fri, Mar 04, 2016 at 14:20:55 +0300, Nikolay Shirokovskiy wrote:
From: ShaoHe Feng <shaohe.feng@intel.com>
Signed-off-by: ShaoHe Feng <shaohe.feng@intel.com> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_monitor.c | 22 +++++++++ src/qemu/qemu_monitor.h | 17 +++++++ src/qemu/qemu_monitor_json.c | 110 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++ 4 files changed, 154 insertions(+)
Looks ok so far. Jirka
On Fri, Mar 04, 2016 at 14:20:55 +0300, Nikolay Shirokovskiy wrote:
From: ShaoHe Feng <shaohe.feng@intel.com>
Signed-off-by: ShaoHe Feng <shaohe.feng@intel.com> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_monitor.c | 22 +++++++++ src/qemu/qemu_monitor.h | 17 +++++++ src/qemu/qemu_monitor_json.c | 110 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++ 4 files changed, 154 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5e4461a..21c1df6 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2157,6 +2157,28 @@ qemuMonitorSetMigrationCacheSize(qemuMonitorPtr mon,
int +qemuMonitorGetMigrationParameters(qemuMonitorPtr mon, + qemuMonitorMigrationParametersPtr params) +{ + QEMU_CHECK_MONITOR_JSON(mon); + + return qemuMonitorJSONGetMigrationParameters(mon, params); +} + +int +qemuMonitorSetMigrationParameters(qemuMonitorPtr mon, + qemuMonitorMigrationParametersPtr params) +{ + VIR_DEBUG("level=%d threads=%d dthreads=%d", + params->level, params->threads, params->dthreads); + + QEMU_CHECK_MONITOR_JSON(mon); + + return qemuMonitorJSONSetMigrationParameters(mon, params); +} + + +int qemuMonitorGetMigrationStats(qemuMonitorPtr mon, qemuMonitorMigrationStatsPtr stats) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 28620b5..b28b431 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -469,6 +469,23 @@ int qemuMonitorGetMigrationCacheSize(qemuMonitorPtr mon, int qemuMonitorSetMigrationCacheSize(qemuMonitorPtr mon, unsigned long long cacheSize);
+typedef struct _qemuMonitorMigrationParameters qemuMonitorMigrationParameters; +typedef qemuMonitorMigrationParameters *qemuMonitorMigrationParametersPtr; +struct _qemuMonitorMigrationParameters { + unsigned int level_set : 1; + unsigned int threads_set : 1; + unsigned int dthreads_set : 1; + + int level; + int threads; + int dthreads; +};
Actually, I think the names should correspond to the names used by QEMU to avoid some confusion. Jirka
On 18.03.2016 17:45, Jiri Denemark wrote:
On Fri, Mar 04, 2016 at 14:20:55 +0300, Nikolay Shirokovskiy wrote:
From: ShaoHe Feng <shaohe.feng@intel.com>
Signed-off-by: ShaoHe Feng <shaohe.feng@intel.com> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_monitor.c | 22 +++++++++ src/qemu/qemu_monitor.h | 17 +++++++ src/qemu/qemu_monitor_json.c | 110 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++ 4 files changed, 154 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5e4461a..21c1df6 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2157,6 +2157,28 @@ qemuMonitorSetMigrationCacheSize(qemuMonitorPtr mon,
int +qemuMonitorGetMigrationParameters(qemuMonitorPtr mon, + qemuMonitorMigrationParametersPtr params) +{ + QEMU_CHECK_MONITOR_JSON(mon); + + return qemuMonitorJSONGetMigrationParameters(mon, params); +} + +int +qemuMonitorSetMigrationParameters(qemuMonitorPtr mon, + qemuMonitorMigrationParametersPtr params) +{ + VIR_DEBUG("level=%d threads=%d dthreads=%d", + params->level, params->threads, params->dthreads); + + QEMU_CHECK_MONITOR_JSON(mon); + + return qemuMonitorJSONSetMigrationParameters(mon, params); +} + + +int qemuMonitorGetMigrationStats(qemuMonitorPtr mon, qemuMonitorMigrationStatsPtr stats) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 28620b5..b28b431 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -469,6 +469,23 @@ int qemuMonitorGetMigrationCacheSize(qemuMonitorPtr mon, int qemuMonitorSetMigrationCacheSize(qemuMonitorPtr mon, unsigned long long cacheSize);
+typedef struct _qemuMonitorMigrationParameters qemuMonitorMigrationParameters; +typedef qemuMonitorMigrationParameters *qemuMonitorMigrationParametersPtr; +struct _qemuMonitorMigrationParameters { + unsigned int level_set : 1; + unsigned int threads_set : 1; + unsigned int dthreads_set : 1; + + int level; + int threads; + int dthreads; +};
Actually, I think the names should correspond to the names used by QEMU to avoid some confusion.
Ouch, this reveals some more misdesigned stuff. Look, I put qemuMonitorMigrationParameters into qemuMigrationCompression which is a kind of reverse aggregation. I see two options to make things the proper way here. 1. Rename qemuMonitorMigrationParameters, qemuMonitorSetMigrationParameters etc to add compression to the naming somehow. If actual qemu command will one day be extended to support parameters from different category - well we just add another json monitor command that will get/set the new subset of parametes. 2. Rework code so that we will aggregate all migration parameters into qemuMonitorSetMigrationParameters value and call actual qemu command only once. This way either we pack all compression values into one structure and have code that fills migration parameters value appropriately or we stop keeping compression related parameters in qemuMigrationCompression. I don't like any of these. So i prefer the first option.
Jirka
On Wed, Mar 23, 2016 at 15:34:07 +0300, Nikolay Shirokovskiy wrote:
+typedef struct _qemuMonitorMigrationParameters qemuMonitorMigrationParameters; +typedef qemuMonitorMigrationParameters *qemuMonitorMigrationParametersPtr; +struct _qemuMonitorMigrationParameters { + unsigned int level_set : 1; + unsigned int threads_set : 1; + unsigned int dthreads_set : 1; + + int level; + int threads; + int dthreads; +};
Actually, I think the names should correspond to the names used by QEMU to avoid some confusion.
Ouch, this reveals some more misdesigned stuff. Look, I put qemuMonitorMigrationParameters into qemuMigrationCompression which is a kind of reverse aggregation. I see two options to make things the proper way here.
1. Rename qemuMonitorMigrationParameters, qemuMonitorSetMigrationParameters etc to add compression to the naming somehow. If actual qemu command will one day be extended to support parameters from different category - well we just add another json monitor command that will get/set the new subset of parametes.
2. Rework code so that we will aggregate all migration parameters into qemuMonitorSetMigrationParameters value and call actual qemu command only once. This way either we pack all compression values into one structure and have code that fills migration parameters value appropriately or we stop keeping compression related parameters in qemuMigrationCompression. I don't like any of these.
So i prefer the first option.
Originally, I inclined to some form of option 2, but now I agree option 1 would be better. At least until we have other migration parameters, but we can rethink the design if needed then. Jirka
On Fri, Mar 18, 2016 at 15:45:39 +0100, Jiri Denemark wrote:
On Fri, Mar 04, 2016 at 14:20:55 +0300, Nikolay Shirokovskiy wrote:
From: ShaoHe Feng <shaohe.feng@intel.com>
Signed-off-by: ShaoHe Feng <shaohe.feng@intel.com> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_monitor.c | 22 +++++++++ src/qemu/qemu_monitor.h | 17 +++++++ src/qemu/qemu_monitor_json.c | 110 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++ 4 files changed, 154 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5e4461a..21c1df6 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c
[...]
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 28620b5..b28b431 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -469,6 +469,23 @@ int qemuMonitorGetMigrationCacheSize(qemuMonitorPtr mon, int qemuMonitorSetMigrationCacheSize(qemuMonitorPtr mon, unsigned long long cacheSize);
+typedef struct _qemuMonitorMigrationParameters qemuMonitorMigrationParameters; +typedef qemuMonitorMigrationParameters *qemuMonitorMigrationParametersPtr; +struct _qemuMonitorMigrationParameters { + unsigned int level_set : 1; + unsigned int threads_set : 1; + unsigned int dthreads_set : 1;
Also booleans are preferred in comparison to this approach.
From: Eli Qiao <liyong.qiao@intel.com> Signed-off-by: Eli Qiao <liyong.qiao@intel.com> Signed-off-by: ShaoHe Feng <shaohe.feng@intel.com> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- tests/qemumonitorjsontest.c | 53 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 1be0bee..b31df36 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1593,6 +1593,58 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) } static int +testQemuMonitorJSONqemuMonitorJSONGetMigrationParameters(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt); + qemuMonitorMigrationParameters compression; + int ret = -1; + + if (!test) + return -1; + + if (qemuMonitorTestAddItem(test, "query-migrate-parameters", + "{" + " \"return\": {" + " \"decompress-threads\": 2," + " \"compress-threads\": 8," + " \"compress-level\": 1" + " }" + "}") < 0) { + goto cleanup; + } + + if (qemuMonitorJSONGetMigrationParameters(qemuMonitorTestGetMonitor(test), + &compression) < 0) + goto cleanup; + + if (compression.level != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Invalid decompress-threads: %d, expected 1", + compression.level); + goto cleanup; + } + if (compression.threads != 8) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Invalid decompress-threads: %d, expected 8", + compression.threads); + goto cleanup; + } + if (compression.dthreads != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Invalid decompress-threads: %d, expected 2", + compression.dthreads); + goto cleanup; + } + ret = 0; + + cleanup: + qemuMonitorTestFree(test); + return ret; +} + + +static int testQemuMonitorJSONqemuMonitorJSONGetMigrationCacheSize(const void *data) { virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; @@ -2333,6 +2385,7 @@ mymain(void) DO_TEST(qemuMonitorJSONGetBlockInfo); DO_TEST(qemuMonitorJSONGetBlockStatsInfo); DO_TEST(qemuMonitorJSONGetMigrationCacheSize); + DO_TEST(qemuMonitorJSONGetMigrationParameters); DO_TEST(qemuMonitorJSONGetMigrationStats); DO_TEST(qemuMonitorJSONGetChardevInfo); DO_TEST(qemuMonitorJSONSetBlockIoThrottle); -- 1.8.3.1
On Fri, Mar 04, 2016 at 14:20:56 +0300, Nikolay Shirokovskiy wrote:
From: Eli Qiao <liyong.qiao@intel.com>
Signed-off-by: Eli Qiao <liyong.qiao@intel.com> Signed-off-by: ShaoHe Feng <shaohe.feng@intel.com> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- tests/qemumonitorjsontest.c | 53 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
Looks OK. Jirka
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- include/libvirt/libvirt-domain.h | 33 ++++++++++++++++ src/qemu/qemu_migration.c | 85 +++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_migration.h | 7 ++++ 3 files changed, 124 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index b3a176f..59df373 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -784,6 +784,39 @@ typedef enum { # define VIR_MIGRATE_PARAM_COMPRESSION "compression" +/** + * VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL: + * + * virDomainMigrate* params field: the level of compression for multithread + * compression as VIR_TYPED_PARAM_INT. Accepted values * are in range 0-9. + * 0 is no compression, 1 is maximum speed and 9 is maximum compression. + */ +# define VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL "compression.mt.level" + +/** + * VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS: + * + * virDomainMigrate* params field: the number of compression threads for + * multithread compression as VIR_TYPED_PARAM_INT. + */ +# define VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS "compression.mt.threads" + +/** + * VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS: + * + * virDomainMigrate* params field: the number of decompression threads for + * multithread compression as VIR_TYPED_PARAM_INT. + */ +# define VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS "compression.mt.dthreads" + +/** + * VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE: + * + * virDomainMigrate* params field: the size of page cache for xbzrle + * compression as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE "compression.xbzrle.cache" + /* Domain migration. */ virDomainPtr virDomainMigrate (virDomainPtr domain, virConnectPtr dconn, unsigned long flags, const char *dname, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 5fcf132..88d62a9 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3305,6 +3305,9 @@ qemuMigrationSetCompression(virQEMUDriverPtr driver, qemuMigrationCompressionPtr compression, unsigned int flags) { + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + /* * if compression methods are not set explicitly use flags to * set default compression methods @@ -3328,7 +3331,24 @@ qemuMigrationSetCompression(virQEMUDriverPtr driver, job) < 0) return -1; - return 0; + if (qemuDomainObjEnterMonitorAsync(driver, vm, job) < 0) + return -1; + + if (qemuMonitorSetMigrationParameters(priv->mon, &compression->params) < 0) + goto cleanup; + + if (compression->xbzrle_cache_set && + qemuMonitorSetMigrationCacheSize(priv->mon, + compression->xbzrle_cache) < 0) + goto cleanup; + + ret = 0; + + cleanup: + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + return ret; } static int @@ -6359,6 +6379,8 @@ qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression, virTypedParameterPtr params, int nparams) { size_t i; + qemuMonitorMigrationParametersPtr cparams = &compression->params; + int rc; for (i = 0; i < nparams; i++) { if (STRNEQ(params[i].field, VIR_MIGRATE_PARAM_COMPRESSION)) @@ -6379,6 +6401,41 @@ qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression, } } +#define VIR_GET_PARAMETER_OPT(PARAM, TYPE, PARENT, VALUE) \ + if ((rc = virTypedParamsGet ## TYPE(params, nparams, \ + PARAM, &PARENT->VALUE)) < 0) \ + return -1; \ + \ + if (rc == 1) \ + PARENT->VALUE ## _set = 1; + + VIR_GET_PARAMETER_OPT(VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL, + Int, cparams, level) + VIR_GET_PARAMETER_OPT(VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS, + Int, cparams, threads) + VIR_GET_PARAMETER_OPT(VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS, + Int, cparams, dthreads) + VIR_GET_PARAMETER_OPT(VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE, + ULLong, compression, xbzrle_cache) + +#undef VIR_GET_PARAMETER_OPT + + if ((cparams->level_set || cparams->threads_set || cparams->dthreads_set) && + !(compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("You cannot specify multithread compression " + "parameters without turning it on.")); + return -1; + } + + if (compression->xbzrle_cache_set && + !(compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("You cannot specify xbzrle compression " + "parameters without turning it on.")); + return -1; + } + return 0; } @@ -6388,6 +6445,8 @@ qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression, int *nparams, int *maxparams) { + qemuMonitorMigrationParametersPtr cparams = &compression->params; + if ((compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE) && virTypedParamsAddString(params, nparams, maxparams, VIR_MIGRATE_PARAM_COMPRESSION, @@ -6400,5 +6459,29 @@ qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression, "multithread") < 0) return -1; + if (cparams->level_set && + virTypedParamsAddInt(params, nparams, maxparams, + VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL, + cparams->level) < 0) + return -1; + + if (cparams->threads_set && + virTypedParamsAddInt(params, nparams, maxparams, + VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS, + cparams->threads) < 0) + return -1; + + if (cparams->dthreads_set && + virTypedParamsAddInt(params, nparams, maxparams, + VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS, + cparams->dthreads) < 0) + return -1; + + if (compression->xbzrle_cache_set && + virTypedParamsAddULLong(params, nparams, maxparams, + VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE, + compression->xbzrle_cache) < 0) + return -1; + return 0; } diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 3cbe944..3758a9c 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -58,6 +58,10 @@ typedef qemuMigrationCompression *qemuMigrationCompressionPtr; VIR_TYPED_PARAM_MULTIPLE, \ VIR_MIGRATE_PARAM_COMPRESSION, VIR_TYPED_PARAM_STRING | \ VIR_TYPED_PARAM_MULTIPLE, \ + VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL, VIR_TYPED_PARAM_INT, \ + VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS, VIR_TYPED_PARAM_INT, \ + VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS, VIR_TYPED_PARAM_INT, \ + VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE, VIR_TYPED_PARAM_ULLONG, \ NULL @@ -84,6 +88,9 @@ typedef enum { struct _qemuMigrationCompression { qemuMigrationCompressMethods methods; + qemuMonitorMigrationParameters params; + unsigned int xbzrle_cache_set : 1; + unsigned long long xbzrle_cache; }; int qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression, -- 1.8.3.1
On Fri, Mar 04, 2016 at 14:20:57 +0300, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- include/libvirt/libvirt-domain.h | 33 ++++++++++++++++ src/qemu/qemu_migration.c | 85 +++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_migration.h | 7 ++++ 3 files changed, 124 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index b3a176f..59df373 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -784,6 +784,39 @@ typedef enum {
# define VIR_MIGRATE_PARAM_COMPRESSION "compression"
+/** + * VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL: + * + * virDomainMigrate* params field: the level of compression for multithread + * compression as VIR_TYPED_PARAM_INT. Accepted values * are in range 0-9.
Looks like a leftover '*' --------------------------------^
+ * 0 is no compression, 1 is maximum speed and 9 is maximum compression. + */ +# define VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL "compression.mt.level"
Then name of the compression method in individual parameters should match the name passed to VIR_MIGRATE_PARAM_COMPRESSION. That is, either change the method name to "mt" or use "compression.multithread." prefix for additional parameters.
+ +/** + * VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS: + * + * virDomainMigrate* params field: the number of compression threads for + * multithread compression as VIR_TYPED_PARAM_INT. + */ +# define VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS "compression.mt.threads" + +/** + * VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS: + * + * virDomainMigrate* params field: the number of decompression threads for + * multithread compression as VIR_TYPED_PARAM_INT. + */ +# define VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS "compression.mt.dthreads" + +/** + * VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE: + * + * virDomainMigrate* params field: the size of page cache for xbzrle + * compression as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE "compression.xbzrle.cache" + /* Domain migration. */ virDomainPtr virDomainMigrate (virDomainPtr domain, virConnectPtr dconn, unsigned long flags, const char *dname, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 5fcf132..88d62a9 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c ... @@ -6379,6 +6401,41 @@ qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression, } }
+#define VIR_GET_PARAMETER_OPT(PARAM, TYPE, PARENT, VALUE) \
The name of this macro is long while still generic. Since it is a local macro, I think you can use a short name, such as GET_PARAM.
+ if ((rc = virTypedParamsGet ## TYPE(params, nparams, \ + PARAM, &PARENT->VALUE)) < 0) \ + return -1; \ + \ + if (rc == 1) \ + PARENT->VALUE ## _set = 1;
Anyway, the macro needs to be enclosed in do { } while () so that you can use semicolon when calling it.
+ + VIR_GET_PARAMETER_OPT(VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL, + Int, cparams, level)
Semicolon should go here ------------------------^
+ VIR_GET_PARAMETER_OPT(VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS, + Int, cparams, threads) + VIR_GET_PARAMETER_OPT(VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS, + Int, cparams, dthreads) + VIR_GET_PARAMETER_OPT(VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE, + ULLong, compression, xbzrle_cache) + +#undef VIR_GET_PARAMETER_OPT + + if ((cparams->level_set || cparams->threads_set || cparams->dthreads_set) && + !(compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("You cannot specify multithread compression " + "parameters without turning it on."));
We don't use '.' at the end of error messages. And I'd also avoid pointing at the user with "You".
+ return -1; + } + + if (compression->xbzrle_cache_set && + !(compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("You cannot specify xbzrle compression " + "parameters without turning it on."));
Ditto.
+ return -1; + } + return 0; }
... Jirka
From: ShaoHe Feng <shaohe.feng@intel.com> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- tools/virsh-domain.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 25 ++++++++++++---- 2 files changed, 103 insertions(+), 5 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 979f115..6dd8eca 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9666,6 +9666,31 @@ static const vshCmdOptDef opts_migrate[] = { .type = VSH_OT_STRING, .help = N_("comma separated list of disks to be migrated") }, + {.name = "compression-method", + .type = VSH_OT_STRING, + .help = N_("comma separated list of compression method to be used") + }, + {.name = "compression-mt-level", + .type = VSH_OT_INT, + .help = N_("compress level for multithread compression. " + "Values are in range 0-9, 9 means maximum compression. " + "'multithread' compression method must be selected with this option.") + }, + {.name = "compression-mt-threads", + .type = VSH_OT_INT, + .help = N_("number of compession threads from multithread compression. " + "'multithread' compression method must be selected with this option.") + }, + {.name = "compression-mt-dthreads", + .type = VSH_OT_INT, + .help = N_("number of decompession threads from multithread compression. " + "'multithread' compression method must be selected with this option.") + }, + {.name = "compression-xbzrle-cache", + .type = VSH_OT_INT, + .help = N_("page cache size for xbzrle compression. " + "'xbzrle' compression method must be selected with this option.") + }, {.name = NULL} }; @@ -9684,6 +9709,9 @@ doMigrate(void *opaque) virTypedParameterPtr params = NULL; int nparams = 0; int maxparams = 0; + int intOpt = 0; + unsigned long long ullOpt = 0; + int rv; virConnectPtr dconn = data->dconn; sigemptyset(&sigmask); @@ -9744,6 +9772,61 @@ doMigrate(void *opaque) VIR_FREE(val); } + if (vshCommandOptStringReq(ctl, cmd, "compression-method", &opt) < 0) + goto out; + if (opt) { + char **val = NULL; + + val = virStringSplit(opt, ",", 0); + + if (virTypedParamsAddStringList(¶ms, + &nparams, + &maxparams, + VIR_MIGRATE_PARAM_COMPRESSION, + (const char **)val) < 0) { + VIR_FREE(val); + goto save_error; + } + + VIR_FREE(val); + } + + if ((rv = vshCommandOptInt(ctl, cmd, "compression-mt-level", &intOpt)) < 0) { + goto save_error; + } else if (rv > 0) { + if (virTypedParamsAddInt(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL, + intOpt) < 0) + goto save_error; + } + + if ((rv = vshCommandOptInt(ctl, cmd, "compression-mt-threads", &intOpt)) < 0) { + goto save_error; + } else if (rv > 0) { + if (virTypedParamsAddInt(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS, + intOpt) < 0) + goto save_error; + } + + if ((rv = vshCommandOptInt(ctl, cmd, "compression-mt-dthreads", &intOpt)) < 0) { + goto save_error; + } else if (rv > 0) { + if (virTypedParamsAddInt(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS, + intOpt) < 0) + goto save_error; + } + + if ((rv = vshCommandOptULongLong(ctl, cmd, "compression-xbzrle-cache", &ullOpt)) < 0) { + goto save_error; + } else if (rv > 0) { + if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE, + ullOpt) < 0) + goto save_error; + } + if (vshCommandOptStringReq(ctl, cmd, "xml", &opt) < 0) goto out; if (opt) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 4662658..854c7ae 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1532,7 +1532,9 @@ to the I<uri> namespace is displayed instead of being modified. [I<--compressed>] [I<--abort-on-error>] [I<--auto-converge>] I<domain> I<desturi> [I<migrateuri>] [I<graphicsuri>] [I<listen-address>] [I<dname>] [I<--timeout> B<seconds>] [I<--xml> B<file>] -[I<--migrate-disks> B<disk-list>] +[I<--migrate-disks> B<disk-list>] [I<--compression-method> B<method-list>] +[I<--compression-mt-level>] [I<--compression-mt-threads>] [I<--compression-mt-dthreads>] +[I<--compression-xbzrle-cache>] Migrate domain to another host. Add I<--live> for live migration; <--p2p> for peer-2-peer migration; I<--direct> for direct migration; or I<--tunnelled> @@ -1555,10 +1557,14 @@ enforces that no incompatible configuration changes will be made to the domain while the migration is underway; this flag is implicitly enabled when supported by the hypervisor, but can be explicitly used to reject the migration if the hypervisor lacks change protection support. I<--verbose> displays the progress -of migration. I<--compressed> activates compression of memory pages that have -to be transferred repeatedly during live migration. I<--abort-on-error> cancels -the migration if a soft error (for example I/O error) happens during the -migration. I<--auto-converge> forces convergence during live migration. +of migration. I<--compressed> activates default compression method +during migration. I<--abort-on-error> cancels the migration if a soft error +(for example I/O error) happens during the migration. I<--auto-converge> +forces convergence during live migration. I<--compression-methods> +configures compression methods to use during migration in the comma separated +B<method-list> argument. Compression methods are driver specific. +Compression methods can be tuned further using various [--<compression-*>] +options. B<Note>: Individual hypervisors usually do not support all possible types of migration. For example, QEMU does not support direct migration. @@ -1594,6 +1600,15 @@ managed migration. B<Note>: The I<desturi> parameter for normal migration and peer2peer migration has different semantics: +QEMU supports "multithread" and "xbzrle" methods which can be used in any +combination. I<--compression-mt-level> sets compression level for multithread +method. Values are in range from 0 to 9, where 1 is maximum speed and 9 is maximum +compression. I<--compression-mt-threads> and I<--compression-mt-dthreads> set +the number of compress threads on source and the number of decompress threads +on target respectively. I<--compression-xbzrle-cache> sets size of page cache in +bytes for xbzrle method. I<--compressed> turns on "xbzrle" compression method with +current compression parameters. + =over 4 =item * normal migration: the I<desturi> is an address of the target host as -- 1.8.3.1
On Fri, Mar 04, 2016 at 14:20:58 +0300, Nikolay Shirokovskiy wrote:
From: ShaoHe Feng <shaohe.feng@intel.com>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- tools/virsh-domain.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 25 ++++++++++++---- 2 files changed, 103 insertions(+), 5 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 979f115..6dd8eca 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9666,6 +9666,31 @@ static const vshCmdOptDef opts_migrate[] = { .type = VSH_OT_STRING, .help = N_("comma separated list of disks to be migrated") }, + {.name = "compression-method",
Looks a bit long to me, personally I'd prefer "comp-" prefix, feel free to ignore my preference though :-) Anyway, this is a rare exception when it makes sense not to put the new stuff at the end of the list. All the new options should go just after --compressed options so that they are all together.
+ .type = VSH_OT_STRING, + .help = N_("comma separated list of compression method to be used")
s/method/methods/
+ }, + {.name = "compression-mt-level",
I think the options should contain the method name rather than some abbreviated form of it.
+ .type = VSH_OT_INT, + .help = N_("compress level for multithread compression. "
The above description is IMHO enough, the rest can go into virsh man page.
+ "Values are in range 0-9, 9 means maximum compression. " + "'multithread' compression method must be selected with this option.") + }, + {.name = "compression-mt-threads", + .type = VSH_OT_INT, + .help = N_("number of compession threads from multithread compression. "
Same here.
+ "'multithread' compression method must be selected with this option.") + }, + {.name = "compression-mt-dthreads", + .type = VSH_OT_INT, + .help = N_("number of decompession threads from multithread compression. "
Here.
+ "'multithread' compression method must be selected with this option.") + }, + {.name = "compression-xbzrle-cache", + .type = VSH_OT_INT, + .help = N_("page cache size for xbzrle compression. "
And here.
+ "'xbzrle' compression method must be selected with this option.") + }, {.name = NULL} };
@@ -9684,6 +9709,9 @@ doMigrate(void *opaque) virTypedParameterPtr params = NULL; int nparams = 0; int maxparams = 0; + int intOpt = 0; + unsigned long long ullOpt = 0; + int rv; virConnectPtr dconn = data->dconn;
sigemptyset(&sigmask); @@ -9744,6 +9772,61 @@ doMigrate(void *opaque) VIR_FREE(val); }
+ if (vshCommandOptStringReq(ctl, cmd, "compression-method", &opt) < 0) + goto out; + if (opt) { + char **val = NULL; + + val = virStringSplit(opt, ",", 0);
It would be more compact and still readable and clear as char **val = virStringSplit(opt, ",", 0);
+ + if (virTypedParamsAddStringList(¶ms, + &nparams, + &maxparams, + VIR_MIGRATE_PARAM_COMPRESSION, + (const char **)val) < 0) { + VIR_FREE(val); + goto save_error; + } + + VIR_FREE(val); + } + + if ((rv = vshCommandOptInt(ctl, cmd, "compression-mt-level", &intOpt)) < 0) { + goto save_error;
goto out;
+ } else if (rv > 0) { + if (virTypedParamsAddInt(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL, + intOpt) < 0) + goto save_error; + } + + if ((rv = vshCommandOptInt(ctl, cmd, "compression-mt-threads", &intOpt)) < 0) { + goto save_error;
goto out;
+ } else if (rv > 0) { + if (virTypedParamsAddInt(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS, + intOpt) < 0) + goto save_error; + } + + if ((rv = vshCommandOptInt(ctl, cmd, "compression-mt-dthreads", &intOpt)) < 0) { + goto save_error;
goto out;
+ } else if (rv > 0) { + if (virTypedParamsAddInt(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS, + intOpt) < 0) + goto save_error; + } + + if ((rv = vshCommandOptULongLong(ctl, cmd, "compression-xbzrle-cache", &ullOpt)) < 0) { + goto save_error;
goto out;
+ } else if (rv > 0) { + if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, + VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE, + ullOpt) < 0) + goto save_error; + } + if (vshCommandOptStringReq(ctl, cmd, "xml", &opt) < 0) goto out; if (opt) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 4662658..854c7ae 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1532,7 +1532,9 @@ to the I<uri> namespace is displayed instead of being modified. [I<--compressed>] [I<--abort-on-error>] [I<--auto-converge>] I<domain> I<desturi> [I<migrateuri>] [I<graphicsuri>] [I<listen-address>] [I<dname>] [I<--timeout> B<seconds>] [I<--xml> B<file>] -[I<--migrate-disks> B<disk-list>] +[I<--migrate-disks> B<disk-list>] [I<--compression-method> B<method-list>] +[I<--compression-mt-level>] [I<--compression-mt-threads>] [I<--compression-mt-dthreads>] +[I<--compression-xbzrle-cache>]
Migrate domain to another host. Add I<--live> for live migration; <--p2p> for peer-2-peer migration; I<--direct> for direct migration; or I<--tunnelled> @@ -1555,10 +1557,14 @@ enforces that no incompatible configuration changes will be made to the domain while the migration is underway; this flag is implicitly enabled when supported by the hypervisor, but can be explicitly used to reject the migration if the hypervisor lacks change protection support. I<--verbose> displays the progress -of migration. I<--compressed> activates compression of memory pages that have -to be transferred repeatedly during live migration. I<--abort-on-error> cancels -the migration if a soft error (for example I/O error) happens during the -migration. I<--auto-converge> forces convergence during live migration. +of migration. I<--compressed> activates default compression method +during migration.
I'd say something like: --compressed activates compression, the compression method is chosen with --compression-method. When no method is specified, a hypervisor default method will be used.
I<--abort-on-error> cancels the migration if a soft error +(for example I/O error) happens during the migration. I<--auto-converge> +forces convergence during live migration. I<--compression-methods> +configures compression methods to use during migration in the comma separated +B<method-list> argument. Compression methods are driver specific. +Compression methods can be tuned further using various [--<compression-*>] +options.
--compressed and --compression-* options should be documented next to each other and ideally in a separate paragraph.
B<Note>: Individual hypervisors usually do not support all possible types of migration. For example, QEMU does not support direct migration. @@ -1594,6 +1600,15 @@ managed migration. B<Note>: The I<desturi> parameter for normal migration and peer2peer migration has different semantics:
+QEMU supports "multithread" and "xbzrle" methods which can be used in any +combination. I<--compression-mt-level> sets compression level for multithread +method. Values are in range from 0 to 9, where 1 is maximum speed and 9 is maximum +compression. I<--compression-mt-threads> and I<--compression-mt-dthreads> set +the number of compress threads on source and the number of decompress threads +on target respectively. I<--compression-xbzrle-cache> sets size of page cache in +bytes for xbzrle method. I<--compressed> turns on "xbzrle" compression method with +current compression parameters.
This should go into the same place where the options are documented. Jirka
ping On 04.03.2016 14:20, Nikolay Shirokovskiy wrote:
Add means to turn multithread compression on during migration. Add means to pass compression parameters in migration command.
Changes from v3 ===============
1. HMP support is dropped.
2. Switch to a different implementation of setting migration parameters. Version 3 sets all parameters when configuring migration compression. If parameter is not specified explicitly it is set to hardcoded default value. New version set only explicitly specified parameters instead. This has some reproducibility drawbacks which are nevertheless could be overcomed and this issue will be addressed in another series.
3. Unspecified values are presented by bitset fields rather then specific values of parameters itself. This is a bit more tedious implementation to use but if we take it we can pass all parameter values checking to QEMU.
4. Misc minor changes on Jiri comments.
Eli Qiao (1): qemumonitorjsontest: add test for getting multithread compress params
Nikolay Shirokovskiy (2): migration: add compress method option qemu: migration: add compression options
ShaoHe Feng (2): qemu monitor: add multithread compress parameters accessors virsh: add compression options for migration
include/libvirt/libvirt-domain.h | 44 +++++++ src/qemu/qemu_driver.c | 29 ++++- src/qemu/qemu_migration.c | 246 ++++++++++++++++++++++++++++++++++----- src/qemu/qemu_migration.h | 30 +++++ src/qemu/qemu_monitor.c | 24 +++- src/qemu/qemu_monitor.h | 18 +++ src/qemu/qemu_monitor_json.c | 110 +++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 + tests/qemumonitorjsontest.c | 53 +++++++++ tools/virsh-domain.c | 83 +++++++++++++ tools/virsh.pod | 25 +++- 11 files changed, 628 insertions(+), 39 deletions(-)
participants (3)
-
Jiri Denemark -
Nikolay Shirokovskiy -
Peter Krempa