On Fri, Mar 06, 2015 at 09:05:43AM -0500, John Ferlan wrote: I would use iothread_id instead and probably change it to unsigned int as the public API also uses unsigned int for iothread_id. Which reminds me, why not use unsigned int for maplen too as it must be 1 or more? Before pushing the first two patches, please reconsider using unsigned int for maplen. Same here, better to go with iothread_id. ACK Pavel

Add qemuDomainPinIOThread to handle setting the CPU affinity for a specific IOThread Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- include/libvirt/libvirt-domain.h | 9 ++ src/qemu/qemu_driver.c | 221 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 230 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index f2f7eb5..11ffd07 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3218,6 +3218,15 @@ typedef void (*virConnectDomainEventDeviceRemovedCallback)(virConnectPtr conn, # define VIR_DOMAIN_TUNABLE_CPU_EMULATORPIN "cputune.emulatorpin" /** + * VIR_DOMAIN_TUNABLE_CPU_IOTHREADSPIN: + * + * Macro represents formatted pinning for one IOThread specified by id which is + * appended to the parameter name, for example "cputune.iothreadpin1", + * as VIR_TYPED_PARAM_STRING. + */ +# define VIR_DOMAIN_TUNABLE_CPU_IOTHREADSPIN "cputune.iothreadpin%u" + +/** * VIR_DOMAIN_TUNABLE_CPU_CPU_SHARES: * * Macro represents proportional weight of the scheduler used on the diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b37474f..aad08b2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5780,6 +5780,226 @@ qemuDomainGetIOThreadsInfo(virDomainPtr dom, return ret; } +static int +qemuDomainPinIOThread(virDomainPtr dom, + unsigned int iothread_id, + unsigned char *cpumap, + int maplen, + unsigned int flags) +{ + int ret = -1; + virQEMUDriverPtr driver = dom->conn->privateData; + virQEMUDriverConfigPtr cfg = NULL; + virDomainObjPtr vm; + virCapsPtr caps = NULL; + virDomainDefPtr persistentDef = NULL; + virBitmapPtr pcpumap = NULL; + bool doReset = false; + qemuDomainObjPrivatePtr priv; + virDomainVcpuPinDefPtr *newIOThreadsPin = NULL; + size_t newIOThreadsPinNum = 0; + virCgroupPtr cgroup_iothread = NULL; + virObjectEventPtr event = NULL; + char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; + char *str = NULL; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxparams = 0; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + cfg = virQEMUDriverGetConfig(driver); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + priv = vm->privateData; + + if (virDomainPinIOThreadEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Changing affinity for IOThread dynamically is " + "not allowed when CPU placement is 'auto'")); + goto cleanup; + } + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) + goto endjob; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) + persistentDef = vm->def; + + /* Coverity didn't realize that targetDef must be set if we got here. */ + sa_assert(persistentDef); + + if (!(pcpumap = virBitmapNewData(cpumap, maplen))) + goto endjob; + + if (virBitmapIsAllClear(pcpumap)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Empty iothread cpumap list for pinning")); + goto endjob; + } + + /* pinning to all physical cpus means resetting, + * so check if we can reset setting. + */ + if (virBitmapIsAllSet(pcpumap)) + doReset = true; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + + if (priv->iothreadpids == NULL) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("IOThread affinity is not supported")); + goto endjob; + } + + if (iothread_id > priv->niothreadpids) { + virReportError(VIR_ERR_INVALID_ARG, + _("iothread value out of range %d > %d"), + iothread_id, priv->niothreadpids); + goto endjob; + } + + if (vm->def->cputune.iothreadspin) { + /* The VcpuPinDefCopy works for IOThreads too */ + newIOThreadsPin = + virDomainVcpuPinDefCopy(vm->def->cputune.iothreadspin, + vm->def->cputune.niothreadspin); + if (!newIOThreadsPin) + goto endjob; + + newIOThreadsPinNum = vm->def->cputune.niothreadspin; + } else { + if (VIR_ALLOC(newIOThreadsPin) < 0) + goto endjob; + newIOThreadsPinNum = 0; + } + + if (virDomainIOThreadsPinAdd(&newIOThreadsPin, &newIOThreadsPinNum, + cpumap, maplen, iothread_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to update iothreadspin")); + virDomainVcpuPinDefArrayFree(newIOThreadsPin, newIOThreadsPinNum); + goto endjob; + } + + /* Configure the corresponding cpuset cgroup before set affinity. */ + if (virCgroupHasController(priv->cgroup, + VIR_CGROUP_CONTROLLER_CPUSET)) { + if (virCgroupNewIOThread(priv->cgroup, iothread_id, + false, &cgroup_iothread) < 0) + goto endjob; + if (qemuSetupCgroupIOThreadsPin(cgroup_iothread, + newIOThreadsPin, + newIOThreadsPinNum, + iothread_id) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("failed to set cpuset.cpus in cgroup" + " for iothread %d"), iothread_id); + goto endjob; + } + } else { + if (virProcessSetAffinity(priv->iothreadpids[iothread_id - 1], + pcpumap) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to set cpu affinity for IOThread %d"), + iothread_id); + goto endjob; + } + } + + if (doReset) { + virDomainIOThreadsPinDel(vm->def, iothread_id); + } else { + if (vm->def->cputune.iothreadspin) + virDomainVcpuPinDefArrayFree(vm->def->cputune.iothreadspin, + vm->def->cputune.niothreadspin); + + vm->def->cputune.iothreadspin = newIOThreadsPin; + vm->def->cputune.niothreadspin = newIOThreadsPinNum; + newIOThreadsPin = NULL; + } + + if (newIOThreadsPin) + virDomainVcpuPinDefArrayFree(newIOThreadsPin, newIOThreadsPinNum); + + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto endjob; + + if (snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH, + VIR_DOMAIN_TUNABLE_CPU_IOTHREADSPIN, iothread_id) < 0) { + goto endjob; + } + + str = virBitmapFormat(pcpumap); + if (virTypedParamsAddString(&eventParams, &eventNparams, + &eventMaxparams, paramField, str) < 0) + goto endjob; + + event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (iothread_id > persistentDef->iothreads) { + virReportError(VIR_ERR_INVALID_ARG, + _("iothread value out of range %d > %d"), + iothread_id, persistentDef->iothreads); + goto endjob; + } + + if (doReset) { + virDomainIOThreadsPinDel(persistentDef, iothread_id); + } else { + if (!persistentDef->cputune.iothreadspin) { + if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) + goto endjob; + persistentDef->cputune.niothreadspin = 0; + } + if (virDomainIOThreadsPinAdd(&persistentDef->cputune.iothreadspin, + &persistentDef->cputune.niothreadspin, + cpumap, + maplen, + iothread_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to update or add iothreadspin xml " + "of a persistent domain")); + goto endjob; + } + } + + ret = virDomainSaveConfig(cfg->configDir, persistentDef); + goto endjob; + } + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + if (cgroup_iothread) + virCgroupFree(&cgroup_iothread); + if (event) + qemuDomainEventQueue(driver, event); + VIR_FREE(str); + virBitmapFree(pcpumap); + qemuDomObjEndAPI(&vm); + virObjectUnref(caps); + virObjectUnref(cfg); + return ret; +} + static int qemuDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr seclabel) { virQEMUDriverPtr driver = dom->conn->privateData; @@ -19328,6 +19548,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainGetVcpus = qemuDomainGetVcpus, /* 0.4.4 */ .domainGetMaxVcpus = qemuDomainGetMaxVcpus, /* 0.4.4 */ .domainGetIOThreadsInfo = qemuDomainGetIOThreadsInfo, /* 1.2.14 */ + .domainPinIOThread = qemuDomainPinIOThread, /* 1.2.14 */ .domainGetSecurityLabel = qemuDomainGetSecurityLabel, /* 0.6.1 */ .domainGetSecurityLabelList = qemuDomainGetSecurityLabelList, /* 0.10.0 */ .nodeGetSecurityModel = qemuNodeGetSecurityModel, /* 0.6.1 */ -- 2.1.0

On 03/09/2015 04:42 PM, Pavel Hrdina wrote: Head scratcher - there's something that caused me to do this, but I forget. In any case, persistentDef isn't used with *_LIVE anyway. I'll move this closer to the _CONFIG option like other uses. Interesting - the vcpupin will have the same issue... I'll have to send a followup patch for that though... Thanks for the reviews. John It seems it's "safe" to just move them then since this isn't a loop

On 03/10/2015 12:37 PM, Ján Tomko wrote: No issues with doing things right and obviously I wasn't aware the current API's are broken... Well let's just say I don't know all the nuances and known issues of cpu pinning... Knowing that the other APIs (vcpu and emulator) are broken - are you planning to send patches to fix them as well? Or is that something I should do mimicing whatever is settled upon here? So, from how I read your comments I have the following diffs which removes the doReset conditions, and removes the PinDel calls as a result of them. Theoretically now nothing calls the PinDel API and while I suppose I could remove it, once the hot remove an IOThread code is added, I'd need it back. With respect to the pin to the domain cpuset - this API takes a new cpumap so I'm not sure I understand under what condition would thus use the domain's cpuset. Maybe I read your comment too literally John diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e3d0acf..74440f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5773,7 +5773,6 @@ qemuDomainPinIOThread(virDomainPtr dom, virCapsPtr caps = NULL; virDomainDefPtr persistentDef = NULL; virBitmapPtr pcpumap = NULL; - bool doReset = false; qemuDomainObjPrivatePtr priv; virDomainVcpuPinDefPtr *newIOThreadsPin = NULL; size_t newIOThreadsPinNum = 0; @@ -5823,12 +5822,6 @@ qemuDomainPinIOThread(virDomainPtr dom, goto endjob; } - /* pinning to all physical cpus means resetting, - * so check if we can reset setting. - */ - if (virBitmapIsAllSet(pcpumap)) - doReset = true; - if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (priv->iothreadpids == NULL) { @@ -5891,17 +5884,13 @@ qemuDomainPinIOThread(virDomainPtr dom, } } - if (doReset) { - virDomainIOThreadsPinDel(vm->def, iothread_id); - } else { - if (vm->def->cputune.iothreadspin) - virDomainVcpuPinDefArrayFree(vm->def->cputune.iothreadspin, - vm->def->cputune.niothreadspin); + if (vm->def->cputune.iothreadspin) + virDomainVcpuPinDefArrayFree(vm->def->cputune.iothreadspin, + vm->def->cputune.niothreadspin); - vm->def->cputune.iothreadspin = newIOThreadsPin; - vm->def->cputune.niothreadspin = newIOThreadsPinNum; - newIOThreadsPin = NULL; - } + vm->def->cputune.iothreadspin = newIOThreadsPin; + vm->def->cputune.niothreadspin = newIOThreadsPinNum; + newIOThreadsPin = NULL; if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob; @@ -5930,24 +5919,20 @@ qemuDomainPinIOThread(virDomainPtr dom, goto endjob; } - if (doReset) { - virDomainIOThreadsPinDel(persistentDef, iothread_id); - } else { - if (!persistentDef->cputune.iothreadspin) { - if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) - goto endjob; - persistentDef->cputune.niothreadspin = 0; - } - if (virDomainIOThreadsPinAdd(&persistentDef->cputune.iothreadspin, - &persistentDef->cputune.niothreadspin, - cpumap, - maplen, - iothread_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add iothreadspin xml " - "of a persistent domain")); + if (!persistentDef->cputune.iothreadspin) { + if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0) goto endjob; - } + persistentDef->cputune.niothreadspin = 0; + } + if (virDomainIOThreadsPinAdd(&persistentDef->cputune.iothreadspin, + &persistentDef->cputune.niothreadspin, + cpumap, + maplen, + iothread_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to update or add iothreadspin xml " + "of a persistent domain")); + goto endjob; } ret = virDomainSaveConfig(cfg->configDir, persistentDef);

https://bugzilla.redhat.com/show_bug.cgi?id=1135491 $ virsh iothread --help NAME iothreadpin - control domain IOThread affinity SYNOPSIS iothreadpin <domain> <iothread> <cpulist> [--config] [--live] [--current] DESCRIPTION Pin domain IOThreads to host physical CPUs. OPTIONS [--domain] <string> domain name, id or uuid [--iothread] <number> IOThread ID number [--cpulist] <string> host cpu number(s) to set --config affect next boot --live affect running domain --current affect current domain Using the output from iothreadsinfo, allow changing the pinned CPUs for a single IOThread. $ virsh iothreadsinfo $dom IOThread ID CPU Affinity --------------------------------------------------- 1 2 2 3 3 0-1 $ virsh iothreadpin $dom 3 0-2 Then view the change $ virsh iothreadsinfo $dom IOThread ID CPU Affinity --------------------------------------------------- 1 2 2 3 3 0-2 If an invalid value is supplied or require option missing, then an error will be displayed: $ virsh iothreadpin $dom 4 3 error: invalid argument: iothread value out of range 4 > 3 $ virsh iothreadpin $dom 3 error: command 'iothreadpin' requires <cpulist> option Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tools/virsh-domain.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 26 +++++++++++++ 2 files changed, 132 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1e9dfe5..1a364bb 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6878,6 +6878,106 @@ cmdIOThreadsInfo(vshControl *ctl, const vshCmd *cmd) } /* + * "iothreadpin" command + */ +static const vshCmdInfo info_iothreadpin[] = { + {.name = "help", + .data = N_("control domain IOThread affinity") + }, + {.name = "desc", + .data = N_("Pin domain IOThreads to host physical CPUs.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_iothreadpin[] = { + {.name = "domain", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("domain name, id or uuid") + }, + {.name = "iothread", + .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ, + .help = N_("IOThread ID number") + }, + {.name = "cpulist", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("host cpu number(s) to set") + }, + {.name = "config", + .type = VSH_OT_BOOL, + .help = N_("affect next boot") + }, + {.name = "live", + .type = VSH_OT_BOOL, + .help = N_("affect running domain") + }, + {.name = "current", + .type = VSH_OT_BOOL, + .help = N_("affect current domain") + }, + {.name = NULL} +}; + +static bool +cmdIOThreadPin(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + const char *cpulist = NULL; + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool current = vshCommandOptBool(cmd, "current"); + unsigned int iothread_id = 0; + int maxcpu; + bool ret = false; + unsigned char *cpumap = NULL; + size_t cpumaplen; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptUInt(cmd, "iothread", &iothread_id) < 0) { + vshError(ctl, "%s", _("iothreadpin: Invalid IOThread number.")); + goto cleanup; + } + + if (vshCommandOptString(cmd, "cpulist", &cpulist) < 0) { + vshError(ctl, "%s", _("iothreadpin: invalid cpulist.")); + goto cleanup; + } + + if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0) + goto cleanup; + cpumaplen = VIR_CPU_MAPLEN(maxcpu); + + /* Pin mode: pinning specified vcpu to specified physical cpus*/ + if (!(cpumap = vshParseCPUList(ctl, cpulist, maxcpu, cpumaplen))) + goto cleanup; + + if (virDomainPinIOThread(dom, iothread_id, + cpumap, cpumaplen, flags) != 0) + goto cleanup; + + ret = true; + + cleanup: + VIR_FREE(cpumap); + virDomainFree(dom); + return ret; +} + +/* * "cpu-compare" command */ static const vshCmdInfo info_cpu_compare[] = { @@ -12786,6 +12886,12 @@ const vshCmdDef domManagementCmds[] = { .info = info_iothreads, .flags = 0 }, + {.name = "iothreadpin", + .handler = cmdIOThreadPin, + .opts = opts_iothreadpin, + .info = info_iothreadpin, + .flags = 0 + }, {.name = "send-key", .handler = cmdSendKey, .opts = opts_send_key, diff --git a/tools/virsh.pod b/tools/virsh.pod index f19601e..9e259cb 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1373,6 +1373,32 @@ a persistent guest. If I<--current> is specified or I<--live> and I<--config> are not specified, then get the IOThread data based on the current guest state. +=item B<iothreadpin> I<domain> I<iothread> I<cpulist> +[[I<--live>] [I<--config>] | [I<--current>]] + +Change the pinning of a domain IOThread to host physical CPUs. In order +to retrieve a list of all IOThreads, use B<iothreadsinfo>. To pin an +I<iothread> specify the I<cpulist> desired for the IOThread ID as listed +in the B<iothreadsinfo> output. + +I<cpulist> is a list of physical CPU numbers. Its syntax is a comma +separated list and a special markup using '-' and '^' (ex. '0-4', '0-3,^2') can +also be allowed. The '-' denotes the range and the '^' denotes exclusive. +If you want to reset iothreadpin setting, that is, to pin an I<iothread> +to all physical cpus, simply specify 'r' as a cpulist. + +If I<--live> is specified, affect a running guest. If the guest is not running, +an error is returned. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified or I<--live> and I<--config> are not specified, +affect the current guest state. +Both I<--live> and I<--config> flags may be given if I<cpulist> is present, +but I<--current> is exclusive. +If no flag is specified, behavior is different depending on hypervisor. + +B<Note>: The expression is sequentially evaluated, so "0-15,^8" is +identical to "9-14,0-7,15" but not identical to "^8,0-15". + =item B<managedsave> I<domain> [I<--bypass-cache>] [{I<--running> | I<--paused>}] [I<--verbose>] -- 2.1.0