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

[Martin Kletzander]

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