[libvirt] [PATCH] Temporarily shutoff CPUSET cgroup in libvirt.

[Tang Chen]

Hi Eric,

On 09/22/2012 03:04 AM, Eric Blake wrote:
>> -#cgroup_controllers = [ "cpu", "devices", "memory", "blkio", "cpuset", "cpuacct" ]
>> +# When a cpu or memory is offlined and onlined again, cgroup will not
>> +# recover any corresponding values. This is a problem in kernel.
>> +# So DO NOT use cpuset cgroup until this problem is fixed in kernel.
>> +#cgroup_controllers = [ "cpu", "devices", "memory", "blkio", "cpuacct" ]
>
> We ought to list which versions of the kernel are known to have the bug
> (that is, say something like 'at least through kernel 3.6'); when we
> finally know which kernel version fixes the bugs in the cpuset
> controller, then we can update the text again to give the user better
> advice on whether they want to be using this.

Sure, I'll add the comment. :)
>
> I'm torn on whether to take this patch prior to 0.10.2 - it makes sense
> to take (that is, the kernel is buggy, and we cause degraded performance
> as a result of the kernel bug any time the system goes through S3), but
> it's rather late, having missed rc2, for such a major change in default
> functionality.  Am I correct, though, that you can manually edit
> /etc/libvirt/qemu.conf to re-enable the use of cpuset even on a buggy
> kernel, for further testing the issues?

Well, I think we'd better make cpuset off as the default functionality.
I like the following idea, adding uname() probe. I will make it possible
to re-enable cpuset for testing purpose and resend a new patch. You may
take the new patch in later version if necessary. :)
>
>>
>>           driver->cgroupControllers =
>>               (1<<  VIR_CGROUP_CONTROLLER_CPU) |
>>               (1<<  VIR_CGROUP_CONTROLLER_DEVICES) |
>>               (1<<  VIR_CGROUP_CONTROLLER_MEMORY) |
>>               (1<<  VIR_CGROUP_CONTROLLER_BLKIO) |
>> -            (1<<  VIR_CGROUP_CONTROLLER_CPUSET) |
>>               (1<<  VIR_CGROUP_CONTROLLER_CPUACCT);
>
> Rather than hard-coding the non-use of cpuset as our default, can we do
> something like a uname() probe or some other clue of whether we have a
> buggy kernel?

As I said above, this is a good idea!
I will add the probe. Thanks. :)
>
>> +++ b/src/util/cgroup.c
>> @@ -543,8 +543,7 @@ static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr group,
>>           /* We need to control cpu bandwidth for each vcpu now */
>>           if ((flags&  VIR_CGROUP_VCPU)&&
>>               (i != VIR_CGROUP_CONTROLLER_CPU&&
>> -             i != VIR_CGROUP_CONTROLLER_CPUACCT&&
>> -             i != VIR_CGROUP_CONTROLLER_CPUSET)) {
>> +             i != VIR_CGROUP_CONTROLLER_CPUACCT)) {
>>               /* treat it as unmounted and we can use virCgroupAddTask */
>>               VIR_FREE(group->controllers[i].mountPoint);
>>               continue;
>
> Is this hunk necessary?  In other words, don't we just gracefully skip
> making per-vcpu groups for any controller that was not mounted, or do we
> loudly fail if all of these controllers are not present?
>