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

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