Re: [libvirt] [PATCH] Temporarily shutoff CPUSET cgroup in libvirt.
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?