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

[Osier Yang]

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