On Mon, Jan 13, 2014 at 14:28:10 +0800, mrhines(a)linux.vnet.ibm.com wrote: I think this would be better as #define VIR_DOMAIN_JOB_THROUGHPUT "throughput" And you need to explicitly mention the type of the value returned in this parameter. Again, the documentation does not mention what type is returned. I think we should take the Mbps value from qemu and turn it into Bps (or even bps), which would make this parameter consistent with other migration statistics parameters that return values in bytes. And then we can change the type to ULLong. Overall the patch looks good and could even be pushed separately. Jirka

On 02/03/2014 08:52 PM, Daniel P. Berrange wrote: Acknowledged. - Michael

From: "Michael R. Hines" <mrhines(a)us.ibm.com&gt; The switch from x-rdma => rdma has not yet happened, but at least we can review the patch until it goes through on qemu-devel. USAGE: $ virsh migrate --live --migrateuri x-rdma:hostname domain qemu+ssh://hostname/system Full documenation of the feature: http://wiki.qemu.org/Features/RDMALiveMigration This patch includes mainly making all the locations in libvirt where the 'tcp' string was hard-coded to be more flexible to use more than one protocol. Signed-off-by: Michael R. Hines <mrhines(a)us.ibm.com&gt; --- src/qemu/qemu_capabilities.c | 13 +++++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 8 ++++ src/qemu/qemu_migration.c | 110 ++++++++++++++++++++++++++++++++++--------- src/qemu/qemu_monitor.c | 3 +- src/qemu/qemu_monitor.h | 1 + src/util/viruri.c | 7 ++- 7 files changed, 119 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 548b988..d82b48c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -243,6 +243,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-mmio", "ich9-intel-hda", "kvm-pit-lost-tick-policy", + + "migrate-qemu-rdma", /* 160 */ ); struct _virQEMUCaps { @@ -906,6 +908,9 @@ virCapsPtr virQEMUCapsInit(virQEMUCapsCachePtr cache) virCapabilitiesAddHostMigrateTransport(caps, "tcp"); + virCapabilitiesAddHostMigrateTransport(caps, + "rdma"); + /* QEMU can support pretty much every arch that exists, * so just probe for them all - we gracefully fail * if a qemu-system-$ARCH binary can't be found @@ -1110,6 +1115,7 @@ virQEMUCapsComputeCmdFlags(const char *help, * -incoming unix (qemu >= 0.12.0) * -incoming fd (qemu >= 0.12.0) * -incoming stdio (all earlier kvm) + * -incoming rdma (qemu >= 2.0.0) * * NB, there was a pre-kvm-79 'tcp' support, but it * was broken, because it blocked the monitor console @@ -1130,6 +1136,9 @@ virQEMUCapsComputeCmdFlags(const char *help, virQEMUCapsSet(qemuCaps, QEMU_CAPS_MIGRATE_KVM_STDIO); } + if (version >= 2000000) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_RDMA); + if (version >= 10000) virQEMUCapsSet(qemuCaps, QEMU_CAPS_0_10); @@ -2561,6 +2570,10 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, if (qemuCaps->version >= 1006000) virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); + if (qemuCaps->version >= 2000000) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_RDMA); + + if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0) goto cleanup; if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0) diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 02d47c6..3e78961 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -198,6 +198,7 @@ enum virQEMUCapsFlags { QEMU_CAPS_DEVICE_VIRTIO_MMIO = 157, /* -device virtio-mmio */ QEMU_CAPS_DEVICE_ICH9_INTEL_HDA = 158, /* -device ich9-intel-hda */ QEMU_CAPS_KVM_PIT_TICK_POLICY = 159, /* kvm-pit.lost_tick_policy */ + QEMU_CAPS_MIGRATE_QEMU_RDMA = 160, /* have qemu rdma migration */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 763417f..0d23d8b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9448,6 +9448,14 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } virCommandAddArg(cmd, migrateFrom); + } else if (STRPREFIX(migrateFrom, "rdma")) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_RDMA)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("RDMA migration is not supported with " + "this QEMU binary")); + goto error; + } + virCommandAddArg(cmd, migrateFrom); } else if (STREQ(migrateFrom, "stdio")) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) { virCommandAddArgFormat(cmd, "fd:%d", migrateFd); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index ef6f1c5..1e0f538 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -46,6 +46,7 @@ #include "virerror.h" #include "viralloc.h" #include "virfile.h" +#include "virprocess.h" #include "datatypes.h" #include "fdstream.h" #include "viruuid.h" @@ -2163,6 +2164,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, virDomainDefPtr *def, const char *origname, virStreamPtr st, + const char *protocol, unsigned short port, bool autoPort, const char *listenAddress, @@ -2275,6 +2277,16 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, freeaddrinfo(info); hostIPv6Capable = true; } + + /* + * RDMA (iWarp) until linux 3.11 is broken, need + * better host librdmacm IPv6 support detection. + * Disallow by default for now if RDMA. + */ + if (hostIPv6Capable && strstr(protocol, "rdma")) { + hostIPv6Capable = false; + } + if (!(qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, (*def)->emulator))) goto cleanup; @@ -2318,9 +2330,9 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, * -incoming <IPv4 addr>:port or -incoming <hostname>:port */ if ((encloseAddress && - virAsprintf(&migrateFrom, "tcp:[%s]:%d", listenAddress, port) < 0) || + virAsprintf(&migrateFrom, "%s:[%s]:%d", protocol, listenAddress, port) < 0) || (!encloseAddress && - virAsprintf(&migrateFrom, "tcp:%s:%d", listenAddress, port) < 0)) + virAsprintf(&migrateFrom, "%s:%s:%d", protocol, listenAddress, port) < 0)) goto cleanup; } @@ -2507,7 +2519,7 @@ qemuMigrationPrepareTunnel(virQEMUDriverPtr driver, ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen, cookieout, cookieoutlen, def, origname, - st, 0, false, NULL, flags); + st, "tcp", 0, false, NULL, flags); return ret; } @@ -2529,6 +2541,8 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, unsigned short port = 0; bool autoPort = true; char *hostname = NULL; + const char *protocol = NULL; + char *well_formed_protocol = NULL; const char *p; char *uri_str = NULL; int ret = -1; @@ -2577,21 +2591,37 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, if (virAsprintf(uri_out, "tcp:%s:%d", hostname, port) < 0) goto cleanup; } else { - /* Check the URI starts with "tcp:". We will escape the + char * protocol_save = NULL; + char * uri_save = NULL; + + /* Check the URI starts with a valid prefix. We will escape the * URI when passing it to the qemu monitor, so bad * characters in hostname part don't matter. */ - if (!(p = STRSKIP(uri_in, "tcp:"))) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("only tcp URIs are supported for KVM/QEMU" - " migrations")); + + if (VIR_STRDUP(uri_save, uri_in) <= 0) { goto cleanup; } - /* Convert uri_in to well-formed URI with // after tcp: */ - if (!(STRPREFIX(uri_in, "tcp://"))) { + protocol = strtok_r(uri_save, ":", &protocol_save); + VIR_FREE(uri_save); + if (protocol) { + if (virAsprintf(&well_formed_protocol, "%s://", protocol) < 0) + goto cleanup; + } + + /* Make sure it's a valid protocol */ + if (!(p = STRSKIP(uri_in, "tcp:")) && + !(p = STRSKIP(uri_in, "rdma:"))) { + virReportError(VIR_ERR_INVALID_ARG, _("URI %s (%s) not supported" + " for KVM/QEMU migrations"), protocol, uri_in); + goto cleanup; + } + + /* Convert uri_in to well-formed URI with // after colon */ + if (!(STRPREFIX(uri_in, well_formed_protocol))) { well_formed_uri = false; - if (virAsprintf(&uri_str, "tcp://%s", p) < 0) + if (virAsprintf(&uri_str, "%s://%s", protocol, p) < 0) goto cleanup; } @@ -2637,10 +2667,13 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen, cookieout, cookieoutlen, def, origname, - NULL, port, autoPort, listenAddress, flags); + NULL, protocol ? protocol : "tcp", + port, autoPort, listenAddress, flags); cleanup: virURIFree(uri); VIR_FREE(hostname); + VIR_FREE(protocol); + VIR_FREE(well_formed_protocol); if (ret != 0) { VIR_FREE(*uri_out); if (autoPort) @@ -2844,6 +2877,7 @@ struct _qemuMigrationSpec { enum qemuMigrationDestinationType destType; union { struct { + const char *proto; const char *name; int port; } host; @@ -3205,6 +3239,7 @@ qemuMigrationRun(virQEMUDriverPtr driver, switch (spec->destType) { case MIGRATION_DEST_HOST: ret = qemuMonitorMigrateToHost(priv->mon, migrate_flags, + spec->dest.host.proto, spec->dest.host.name, spec->dest.host.port); break; @@ -3335,7 +3370,7 @@ cancel: goto cleanup; } -/* Perform migration using QEMU's native TCP migrate support, +/* Perform migration using QEMU's native migrate support, * not encrypted obviously */ static int doNativeMigrate(virQEMUDriverPtr driver, @@ -3353,7 +3388,11 @@ static int doNativeMigrate(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; virURIPtr uribits = NULL; int ret = -1; + char *tmp = NULL; qemuMigrationSpec spec; + char *well_formed_proto = NULL; + char * protocol_save = NULL; + char * uri_save = NULL; VIR_DEBUG("driver=%p, vm=%p, uri=%s, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=%lx, resource=%lu, " @@ -3362,20 +3401,44 @@ static int doNativeMigrate(virQEMUDriverPtr driver, cookieout, cookieoutlen, flags, resource, NULLSTR(graphicsuri)); - if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) { - char *tmp; - /* HACK: source host generates bogus URIs, so fix them up */ - if (virAsprintf(&tmp, "tcp://%s", uri + strlen("tcp:")) < 0) + ret = VIR_STRDUP(uri_save, uri); + if (ret <= 0) { + return -1; + } + + spec.dest.host.proto = strtok_r(uri_save, ":", &protocol_save); + VIR_FREE(uri_save); + + /* HACK: source host generates bogus URIs, so fix them up */ + if (spec.dest.host.proto) { + ret = virAsprintf(&well_formed_proto, "%s://", + spec.dest.host.proto); + if (ret < 0) + return ret; + } + + /* HACK: source host generates bogus URIs, so fix them up */ + + if (!STRPREFIX(uri, well_formed_proto)) { + if (virAsprintf(&tmp, "%s://%s", spec.dest.host.proto, + uri + strlen(spec.dest.host.proto) + 1) < 0) return -1; - uribits = virURIParse(tmp); - VIR_FREE(tmp); } else { uribits = virURIParse(uri); } - if (!uribits) - return -1; - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) + if (tmp) { + uribits = virURIParse(tmp); + VIR_FREE(tmp); + } + + if (!uribits) { + ret = -1; + goto err; + } + + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) && + !STREQ(spec.dest.host.proto, "rdma")) spec.destType = MIGRATION_DEST_CONNECT_HOST; else spec.destType = MIGRATION_DEST_HOST; @@ -3392,6 +3455,9 @@ static int doNativeMigrate(virQEMUDriverPtr driver, virURIFree(uribits); +err: + VIR_FREE(well_formed_proto); + return ret; } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 1514715..5a450e2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2164,6 +2164,7 @@ int qemuMonitorMigrateToFd(qemuMonitorPtr mon, int qemuMonitorMigrateToHost(qemuMonitorPtr mon, unsigned int flags, + const char *proto, const char *hostname, int port) { @@ -2179,7 +2180,7 @@ int qemuMonitorMigrateToHost(qemuMonitorPtr mon, } - if (virAsprintf(&uri, "tcp:%s:%d", hostname, port) < 0) + if (virAsprintf(&uri, "%s:%s:%d", proto, hostname, port) < 0) return -1; if (mon->json) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 27f9cb4..16b0b77 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -476,6 +476,7 @@ int qemuMonitorMigrateToFd(qemuMonitorPtr mon, int qemuMonitorMigrateToHost(qemuMonitorPtr mon, unsigned int flags, + const char *proto, const char *hostname, int port); diff --git a/src/util/viruri.c b/src/util/viruri.c index 35efad8..662029a 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -187,7 +187,12 @@ virURIParse(const char *uri) goto error; /* First check: does it even make sense to jump inside */ - if (ret->server != NULL && + + /* + * IPv6 rdma over iwarp is broken in linux. Waiting for a + * fix on the kernel side... + */ + if (ret->server != NULL && !STREQ(ret->scheme, "rdma") && ret->server[0] == '[') { size_t length = strlen(ret->server); -- 1.8.1.2

On 02/03/2014 08:19 AM, Jiri Denemark wrote: s/documenation/documentation/ Better than hard-coding it to a version string is to probe the results of query-migrate-capabilities and only setting the capability if the resulting list includes rdma-pin-all, as that will serve as a reliable witness of qemu being new enough to support rdma without an x- prefix. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 04/04/2014 12:19 AM, Michael R. Hines wrote: No. Features get backported downstream all the time, to something that ostensibly fails the version number check. For example, RHEL 6 qemu claims to be 0.10, but has backported many features that are much closer to upstream qemu 1.7. Libvirt basing a feature check on a version number will guess wrong if RHEL 7 backports the upstream qemu 2.0 feature to whatever 1.x version of downstream qemu lives in RHEL. Whereas probing for the _feature_ (by calling query-migrate-capabilities and looking for rdma-pin-all) will work for ALL qemu builds, regardless of whether that qemu calls itself 2.0 or not. Getting an error message from qemu about an unsupported option doesn't always read very well - having libvirt query qemu to see if the option is supported, so that libvirt controls the error message when it is not, often leads to a nicer user experience. If rdma-pin-all appears in the query-migrate-capabilities output of a qemu binary compiled without RDMA support, that is a bug in qemu, and should be fixed, preferably before qemu 2.0 is out. The whole point of feature detection is to be a reliable way of learning whether the feature is present and supported. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 02/03/2014 11:19 PM, Jiri Denemark wrote: Acknowledged. Acknowledged. Acknowledged. Acknowledged. Here's the breakdown: IPv6 RDMA over native (Infiniband): Works IPv6 RDMA over RoCE (ethernet): Works IPv6 RDMA over iWarp (ethernet): Does not yet work IPv4 RDMA over anything: Never had a problem As you can see, there's just that one special case (iWarp). As of now, there is no method in Linux to "detect" in a backwards-compatible way whether or not the current kernel version has fixed the bug (either through /sysfs or /proc or some other configurable way to query whether the linux RDMA device drivers can correctly perform the IPv6 connection establishment.) Without such a detection mechanism, there's no safe way to remove this IPv6 check without propogating the QEMU connection failure all the way back to libvirt and reporting it to the user...... And if the user got such an error report, what would they do about it exactly? Shut down libvirt / management software, disable IPv6 so that the error goes away and then restarting the management software? Comments? My apologies. I'll take care of it next time..... Acknowledged...... We still need a solution here, though: This function is not respecting cases where the open IPv6 address "[::]" is not supported - so, because of the bug in the kernel, binding to and connecting to this address will always fail. If we *must* have an IPv4 address listening for RDMA (i.e. '0' or '0.0.0.0'), then we need a way to override the default listening address. Comments? - Michael

On Mon, Jan 13, 2014 at 14:28:12 +0800, mrhines(a)linux.vnet.ibm.com wrote: Hmm, I forgot to ask when I was reviewing the previous patch but does any of this RDMA migration functionality require special privileges or is any unprivileged process able to use RDMA? The URI should be rdma://hostname Bad indentation. Is this capability always present when QEMU supports RDMA migration? If so, it could be used when we check if QEMU supports RDMA migration. Indentation. And the flag should be rejected when protocol is not rdma. I think we don't want to match just a part of the protocol; use STREQ() instead. Apart from weird indentation of the virProcessSetMaxMemLock line, there are several issues here: - this code should be moved inside qemuMigrationSetPinAll and done only if VIR_MIGRATE_RDMA_PIN_ALL flag was used. - virProcessSetMaxMemLock wants the value in bytes, thus memKB should rather by multiplied by 1024. - what memory is going to be mlock()ed with rdma-pin-all? Is it going to be all memory allocated by QEMU or just the domain's memory? If it's all QEMU memory, we already painfully found out it's impossible to automatically compute how much memory QEMU consumes in addition to the configured domain's memory and I think a better approach would be to just migration with rdma-pin-all unless hard_limit is specified. Same comments as for the equivalent hunk in qemuMigrationPrepareAny. Same comments as for the equivalent hunk in qemuMigrationPrepareAny. Jirka