Abstract the codes to prepare cpumap into a helper a function, which can be used later. * src/qemu/qemu_process.h: Declare qemuPrepareCpumap * src/qemu/qemu_process.c: Implement qemuPrepareCpumap, and use it. --- src/qemu/qemu_process.c | 62 +++++++++++++++++++++++++++++++---------------- src/qemu/qemu_process.h | 2 + 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 969e3ce..26be35a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1890,23 +1890,20 @@ qemuGetNumadAdvice(virDomainDefPtr def ATTRIBUTE_UNUSED) } #endif -/* - * To be run between fork/exec of QEMU only +/* Helper to prepare cpumap for affinity setting, convert + * NUMA nodeset into cpuset if @nodemask is not NULL, otherwise + * just return a new allocated bitmap. */ -static int -qemuProcessInitCpuAffinity(struct qemud_driver *driver, - virDomainObjPtr vm, - virBitmapPtr nodemask) +virBitmapPtr +qemuPrepareCpumap(struct qemud_driver *driver, + virBitmapPtr nodemask) { - int ret = -1; int i, hostcpus, maxcpu = QEMUD_CPUMASK_LEN; virNodeInfo nodeinfo; - virBitmapPtr cpumap, cpumapToSet; - - VIR_DEBUG("Setting CPU affinity"); + virBitmapPtr cpumap = NULL; if (nodeGetInfo(NULL, &nodeinfo) < 0) - return -1; + return NULL; /* setaffinity fails if you set bits for CPUs which * aren't present, so we have to limit ourselves */ @@ -1914,34 +1911,57 @@ qemuProcessInitCpuAffinity(struct qemud_driver *driver, if (maxcpu > hostcpus) maxcpu = hostcpus; - cpumap = virBitmapNew(maxcpu); - if (!cpumap) { + if (!(cpumap = virBitmapNew(maxcpu))) { virReportOOMError(); - return -1; + return NULL; } - cpumapToSet = cpumap; - - if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { - VIR_DEBUG("Set CPU affinity with advisory nodeset from numad"); - /* numad returns the NUMA node list, convert it to cpumap */ + if (nodemask) { for (i = 0; i < driver->caps->host.nnumaCell; i++) { int j; int cur_ncpus = driver->caps->host.numaCell[i]->ncpus; bool result; - if (virBitmapGetBit(nodemask, i, &result) < 0) - goto cleanup; + if (virBitmapGetBit(nodemask, i, &result) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to covert nodeset to cpuset")); + virBitmapFree(cpumap); + return NULL; + } if (result) { for (j = 0; j < cur_ncpus; j++) ignore_value(virBitmapSetBit(cpumap, driver->caps->host.numaCell[i]->cpus[j])); } } + } + + return cpumap; +} + +/* + * To be run between fork/exec of QEMU only + */ +static int +qemuProcessInitCpuAffinity(struct qemud_driver *driver, + virDomainObjPtr vm, + virBitmapPtr nodemask) +{ + int ret = -1; + virBitmapPtr cpumap = NULL; + virBitmapPtr cpumapToSet = NULL; + + if (!(cpumap = qemuPrepareCpumap(driver, nodemask))) + return -1; + + if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { + VIR_DEBUG("Set CPU affinity with advisory nodeset from numad"); + cpumapToSet = cpumap; } else { VIR_DEBUG("Set CPU affinity with specified cpuset"); if (vm->def->cpumask) { cpumapToSet = vm->def->cpumask; } else { + cpumapToSet = cpumap; /* You may think this is redundant, but we can't assume libvirtd * itself is running on all pCPUs, so we need to explicitly set * the spawned QEMU instance to all pCPUs if no map is given in diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 38edde7..543c9ee 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -96,5 +96,7 @@ int qemuProcessAutoDestroyRemove(struct qemud_driver *driver, virDomainObjPtr vm); bool qemuProcessAutoDestroyActive(struct qemud_driver *driver, virDomainObjPtr vm); +virBitmapPtr qemuPrepareCpumap(struct qemud_driver *driver, + virBitmapPtr nodemask); #endif /* __QEMU_PROCESS_H__ */ -- 1.7.7.6

When the cpu placement model is "auto", it sets the affinity for domain process with the advisory nodeset from numad, however, creating cgroup for the domain process (called emulator thread in some contexts) later overrides that with pinning it to all available pCPUs. How to reproduce: * Configure the domain with "auto" placement for <vcpu>, e.g. <vcpu placement='auto'>4</vcpu> * % virsh start dom * % cat /proc/$dompid/status Though the emulator cgroup cause conflicts, but we can't simply prohibit creating it, as other tunables are still useful, such as "emulator_period", which is used by API virDomainSetSchedulerParameter. So this patch doesn't prohibit creating the emulator cgroup, but inherit the nodeset from numad, and reset the affinity for domain process. * src/qemu/qemu_cgroup.h: Modify definition of qemuSetupCgroupForEmulator to accept the passed nodenet * src/qemu/qemu_cgroup.c: Set the affinity with the passed nodeset --- src/qemu/qemu_cgroup.c | 17 ++++++++++++++--- src/qemu/qemu_cgroup.h | 3 ++- src/qemu/qemu_process.c | 2 +- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index db371a0..8e99c01 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -25,6 +25,7 @@ #include "qemu_cgroup.h" #include "qemu_domain.h" +#include "qemu_process.h" #include "cgroup.h" #include "logging.h" #include "memory.h" @@ -637,9 +638,11 @@ cleanup: } int qemuSetupCgroupForEmulator(struct qemud_driver *driver, - virDomainObjPtr vm) + virDomainObjPtr vm, + virBitmapPtr nodemask) { virBitmapPtr cpumask = NULL; + virBitmapPtr cpumap = NULL; virCgroupPtr cgroup = NULL; virCgroupPtr cgroup_emulator = NULL; virDomainDefPtr def = vm->def; @@ -687,10 +690,15 @@ int qemuSetupCgroupForEmulator(struct qemud_driver *driver, } } - if (def->cputune.emulatorpin) + if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { + if (!(cpumap = qemuPrepareCpumap(driver, nodemask))) + goto cleanup; + cpumask = cpumap; + } else if (def->cputune.emulatorpin) { cpumask = def->cputune.emulatorpin->cpumask; - else if (def->cpumask) + } else if (def->cpumask) { cpumask = def->cpumask; + } if (cpumask) { if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPUSET)) { @@ -698,6 +706,9 @@ int qemuSetupCgroupForEmulator(struct qemud_driver *driver, if (rc < 0) goto cleanup; } + + if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) + virBitmapFree(cpumask); cpumask = NULL; /* sanity */ } diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index c552162..50ee092 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -58,7 +58,8 @@ int qemuSetupCgroupVcpuPin(virCgroupPtr cgroup, int qemuSetupCgroupEmulatorPin(virCgroupPtr cgroup, virBitmapPtr cpumask); int qemuSetupCgroupForVcpu(struct qemud_driver *driver, virDomainObjPtr vm); int qemuSetupCgroupForEmulator(struct qemud_driver *driver, - virDomainObjPtr vm); + virDomainObjPtr vm, + virBitmapPtr nodemask); int qemuRemoveCgroup(struct qemud_driver *driver, virDomainObjPtr vm, int quiet); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 26be35a..5004e9b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3812,7 +3812,7 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; VIR_DEBUG("Setting cgroup for emulator (if required)"); - if (qemuSetupCgroupForEmulator(driver, vm) < 0) + if (qemuSetupCgroupForEmulator(driver, vm, nodemask) < 0) goto cleanup; VIR_DEBUG("Setting VCPU affinities"); -- 1.7.7.6

On 2012年10月24日 21:17, Martin Kletzander wrote: Ah, okay. I will squash the following in: diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 1754d6f..8f8ef08 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -706,9 +706,6 @@ int qemuSetupCgroupForEmulator(struct qemud_driver *driver, if (rc < 0) goto cleanup; } - - if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) - virBitmapFree(cpumask); cpumask = NULL; /* sanity */ } @@ -725,6 +722,8 @@ int qemuSetupCgroupForEmulator(struct qemud_driver *driver, return 0; cleanup: + virBitmapFree(cpumap); + if (cgroup_emulator) { virCgroupRemove(cgroup_emulator); virCgroupFree(&cgroup_emulator);

On 2012年10月24日 21:36, Martin Kletzander wrote: Sigh, though rushing is always not good, but I will rush again. diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 1754d6f..5ce748a 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -706,9 +706,6 @@ int qemuSetupCgroupForEmulator(struct qemud_driver *driver, if (rc < 0) goto cleanup; } - - if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) - virBitmapFree(cpumask); cpumask = NULL; /* sanity */ } @@ -722,9 +719,12 @@ int qemuSetupCgroupForEmulator(struct qemud_driver *driver, virCgroupFree(&cgroup_emulator); virCgroupFree(&cgroup); + virBitmapFree(cpumap); return 0; cleanup: + virBitmapFree(cpumap); + if (cgroup_emulator) { virCgroupRemove(cgroup_emulator); virCgroupFree(&cgroup_emulator);

On one hand, numad probably will manage the affinity of domain process dynamically in future. On the other hand, even numad won't manage it, it still could confusion. Let's make things simpler enough to avoid the lair for now. --- src/qemu/qemu_driver.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8af316f..254f191 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4204,6 +4204,13 @@ qemudDomainPinEmulator(virDomainPtr dom, goto cleanup; } + if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Changing affinity for emulator thread dynamically " + "is not allowed when CPU placement is 'auto'")); + goto cleanup; + } + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) goto cleanup; -- 1.7.7.6

On 2012年10月24日 21:38, Martin Kletzander wrote: IMO no need to disable for vCPU threads, as "auto" placement only cares about the affinity of domain process, it might imply we should change that, but now it is that. However, then decision whether we should do it at all

On 10/24/2012 03:45 PM, Osier Yang wrote: Of course, sorry, my bad. ACK, of course. Martin