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

[Martin Kletzander]

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