On Fri, Mar 04, 2011 at 12:06:26PM +0800, Hu Tao wrote:

If any cgroup controller is not avaiable, libvirt disables other controllers as well although they are avaiable. This patch enables cgroup controllers as much as possible.

the kernel shipped with RHEL6 doesn't support mutli-level directory for blkio controller, this causes all other controllers disabled by libvirt and virsh memtune, schedinfo doesn't work.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- src/util/cgroup.c | 12 ++++++++++-- 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/util/cgroup.c b/src/util/cgroup.c index c5b8cdd..30b6dd8 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -507,6 +507,7 @@ static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr group, { int i; int rc = 0; + int n = 0;

VIR_DEBUG("Make group %s", group->path); for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) { @@ -529,7 +530,7 @@ static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr group, mkdir(path, 0755) < 0) { rc = -errno; VIR_FREE(path); - break; + continue;

IMHO, we should only 'continue' if it was the blkio controller. All the others should treat mkdir failure as a fatal problem. Also if we 'continue' we should not set 'rc = -errno'....

} if (group->controllers[VIR_CGROUP_CONTROLLER_CPUSET].mountPoint != NULL && (i == VIR_CGROUP_CONTROLLER_CPUSET || @@ -556,9 +557,12 @@ static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr group, } }

+ n++; VIR_FREE(path); }

+ if (n > 0) + return 0; return rc;

..then this addition is not required.

}

@@ -743,6 +747,7 @@ int virCgroupRemove(virCgroupPtr group) int virCgroupAddTask(virCgroupPtr group, pid_t pid) { int rc = 0; + int n = 0; int i;

for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) { @@ -752,9 +757,12 @@ int virCgroupAddTask(virCgroupPtr group, pid_t pid)

rc = virCgroupSetValueU64(group, i, "tasks", (unsigned long long)pid); if (rc != 0) - break; + continue; + n++; }

+ if (n > 0) + return 0; return rc; }

Again, only do this for the 'blkio' controller and set 'rc' back to 0 if we are ignoring the error. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 03/06/2011 08:49 PM, Hu Tao wrote:

This patch enables cgroup controllers as much as possible by skipping the creation of blkio controller when running with old kenels that

s/kenels/kernels/

doesn't support multi-level directory for blkio controller.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- src/util/cgroup.c | 28 +++++++++++++++++++++++----- 1 files changed, 23 insertions(+), 5 deletions(-)

@@ -527,9 +527,19 @@ static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr group, if (access(path, F_OK) != 0) { if (!create || mkdir(path, 0755) < 0) { - rc = -errno; - VIR_FREE(path); - break; + /* With a kernel that doesn't support multi-level directory + * for blkio controller, libvirt will fail and disable all + * other controllers even though they are available. So skip + * blkio here if mkdir fails. */ + if (i == VIR_CGROUP_CONTROLLER_BLKIO) { + rc = 0; + VIR_FREE(path); + continue; + } else { + rc = -errno; + VIR_FREE(path); + break; + } } if (group->controllers[VIR_CGROUP_CONTROLLER_CPUSET].mountPoint != NULL && (i == VIR_CGROUP_CONTROLLER_CPUSET || @@ -751,8 +761,16 @@ int virCgroupAddTask(virCgroupPtr group, pid_t pid) continue;

rc = virCgroupSetValueU64(group, i, "tasks", (unsigned long long)pid); - if (rc != 0) - break; + + /* See virCgroupMakeGroup() for the reason why check BLKIO here */

s/why check/why we special case/ But I don't like this hunk in the first place - it means that if you have a newer kernel that does support subdirectories, you ignore the write failure to add the process into the group. Rather, it's easier to just nuke the mount point once we detect the problem (by pretending that blkio is not mounted, we don't ever try the SetValueU64 in the first place on an old kernel, while a new kernel doesn't need any code change here). Here's what I squashed in before pushing just the first hunk: diff --git i/src/util/cgroup.c w/src/util/cgroup.c index d3855c4..afe8731 100644 --- i/src/util/cgroup.c +++ w/src/util/cgroup.c @@ -529,10 +529,11 @@ static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr group, mkdir(path, 0755) < 0) { /* With a kernel that doesn't support multi-level directory * for blkio controller, libvirt will fail and disable all - * other controllers even though they are available. So skip - * blkio here if mkdir fails. */ + * other controllers even though they are available. So + * treat blkio as unmounted if mkdir fails. */ if (i == VIR_CGROUP_CONTROLLER_BLKIO) { rc = 0; + VIR_FREE(group->controllers[i].mountPoint); VIR_FREE(path); continue; } else { -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org