[libvirt] [PATCH 0/3] Do not override affinity set by auto placement
Creating cgroup for emulator thread will override the affinity set by "auto" placement with all available pCPUs. 1/3 is just to add a helper to for later use. 2/3 to keep the affinity set by 'auto' placement. 3/3 to prohibit changing the affinity for emulator thread dynamically if placement == "auto". Osier Yang (3): qemu: Add helper to prepare cpumap for affinity setting qemu: Keep the affinity when creating cgroup for emulator thread qemu: Prohibit chaning affinity of domain process if placement is 'auto' src/qemu/qemu_cgroup.c | 17 ++++++++++-- src/qemu/qemu_cgroup.h | 3 +- src/qemu/qemu_driver.c | 7 +++++ src/qemu/qemu_process.c | 64 ++++++++++++++++++++++++++++++---------------- src/qemu/qemu_process.h | 2 + 5 files changed, 67 insertions(+), 26 deletions(-) -- 1.7.7.6
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
On 10/24/2012 12:00 PM, Osier Yang wrote:
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(-)
ACK, Martin
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 10/24/2012 12:00 PM, Osier Yang wrote:
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 @@ -698,6 +706,9 @@ int qemuSetupCgroupForEmulator(struct qemud_driver *driver, if (rc < 0) goto cleanup;
In case you go to the cleanup here, cpumask is not free()'d.
} + + if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) + virBitmapFree(cpumask);
You can free cpumap here or at the end and cleanup as well, no need for checking the placement then.
cpumask = NULL; /* sanity */ }
Martin
On 2012年10月24日 21:17, Martin Kletzander wrote:
On 10/24/2012 12:00 PM, Osier Yang wrote:
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 @@ -698,6 +706,9 @@ int qemuSetupCgroupForEmulator(struct qemud_driver *driver, if (rc< 0) goto cleanup;
In case you go to the cleanup here, cpumask is not free()'d.
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 10/24/2012 03:29 PM, Osier Yang wrote:
On 2012年10月24日 21:17, Martin Kletzander wrote:
On 10/24/2012 12:00 PM, Osier Yang wrote:
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 @@ -698,6 +706,9 @@ int qemuSetupCgroupForEmulator(struct qemud_driver *driver, if (rc< 0) goto cleanup;
In case you go to the cleanup here, cpumask is not free()'d.
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);
Not quite the right thing to do, cleanup is not done if it is successful, you have to _add_ it to cleanup or reorganize it :) Martin
On 2012年10月24日 21:36, Martin Kletzander wrote:
On 10/24/2012 03:29 PM, Osier Yang wrote:
On 2012年10月24日 21:17, Martin Kletzander wrote:
On 10/24/2012 12:00 PM, Osier Yang wrote:
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 @@ -698,6 +706,9 @@ int qemuSetupCgroupForEmulator(struct qemud_driver *driver, if (rc< 0) goto cleanup;
In case you go to the cleanup here, cpumask is not free()'d.
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);
Not quite the right thing to do, cleanup is not done if it is successful, you have to _add_ it to cleanup or reorganize it :)
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 10/24/2012 03:41 PM, Osier Yang wrote:
On 2012年10月24日 21:36, Martin Kletzander wrote:
On 10/24/2012 03:29 PM, Osier Yang wrote:
On 2012年10月24日 21:17, Martin Kletzander wrote:
On 10/24/2012 12:00 PM, Osier Yang wrote:
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 @@ -698,6 +706,9 @@ int qemuSetupCgroupForEmulator(struct qemud_driver *driver, if (rc< 0) goto cleanup;
In case you go to the cleanup here, cpumask is not free()'d.
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);
Not quite the right thing to do, cleanup is not done if it is successful, you have to _add_ it to cleanup or reorganize it :)
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);
This one looks clean, ACK =) Martin
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 10/24/2012 12:00 PM, Osier Yang wrote:
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,
s/even/even if/
it still could confusion. Let's make things simpler enough to avoid
s/could/could cause/
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;
We should unify if the vcpu pinning is only meant for starting the domain or if it's mandatory for the whole time domain is running. It is possible now to have auto placement and pin the vcpus while the domain is running. If we want to disable it for emulator threads, we should also do it for VCPU threads. However, then decision whether we should do it at all should not be made by me. Anyone else has an opinion on that? Martin
On 2012年10月24日 21:38, Martin Kletzander wrote:
On 10/24/2012 12:00 PM, Osier Yang wrote:
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,
s/even/even if/
it still could confusion. Let's make things simpler enough to avoid
s/could/could cause/
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;
We should unify if the vcpu pinning is only meant for starting the domain or if it's mandatory for the whole time domain is running. It is possible now to have auto placement and pin the vcpus while the domain is running.
If we want to disable it for emulator threads, we should also do it for VCPU threads.
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
should not be made by me. Anyone else has an opinion on that?
Martin
On 2012年10月24日 21:45, Osier Yang wrote:
On 2012年10月24日 21:38, Martin Kletzander wrote:
On 10/24/2012 12:00 PM, Osier Yang wrote:
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,
s/even/even if/
it still could confusion. Let's make things simpler enough to avoid
s/could/could cause/
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;
We should unify if the vcpu pinning is only meant for starting the domain or if it's mandatory for the whole time domain is running. It is possible now to have auto placement and pin the vcpus while the domain is running.
If we want to disable it for emulator threads, we should also do it for VCPU threads.
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.
I'm fine to keep this patch in the list though, it's not that hurry to get it in.
However, then decision whether we should do it at all
should not be made by me. Anyone else has an opinion on that?
Martin
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 10/24/2012 03:45 PM, Osier Yang wrote:
On 2012年10月24日 21:38, Martin Kletzander wrote:
On 10/24/2012 12:00 PM, Osier Yang wrote:
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,
s/even/even if/
it still could confusion. Let's make things simpler enough to avoid
s/could/could cause/
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;
We should unify if the vcpu pinning is only meant for starting the domain or if it's mandatory for the whole time domain is running. It is possible now to have auto placement and pin the vcpus while the domain is running.
If we want to disable it for emulator threads, we should also do it for VCPU threads.
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
should not be made by me. Anyone else has an opinion on that?
Martin
Of course, sorry, my bad. ACK, of course. Martin
On 2012年10月24日 21:54, Martin Kletzander wrote:
On 10/24/2012 03:45 PM, Osier Yang wrote:
On 2012年10月24日 21:38, Martin Kletzander wrote:
On 10/24/2012 12:00 PM, Osier Yang wrote:
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,
s/even/even if/
it still could confusion. Let's make things simpler enough to avoid
s/could/could cause/
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;
We should unify if the vcpu pinning is only meant for starting the domain or if it's mandatory for the whole time domain is running. It is possible now to have auto placement and pin the vcpus while the domain is running.
If we want to disable it for emulator threads, we should also do it for VCPU threads.
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
should not be made by me. Anyone else has an opinion on that?
Martin
Of course, sorry, my bad. ACK, of course.
Thanks for the reviewing, since the patches are not likely to break things. I pushed them now. Regards, Osier
participants (2)
-
Martin Kletzander -
Osier Yang