Re: [libvirt] [PATCH 2/3] qemu: Keep the affinity when creating cgroup for emulator thread

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);