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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list