[libvirt] [PATCH] qemu: don't error out when cgroups don't exist
When creating cgroups for vcpu and emulator threads whilst starting a domain, we explicitly skip creating those cgroups in case priv->cgroup is NULL (cgroups not supported) because SetAffinity() serves the same purpose. If the host supports only some cgroups (the ones we need are either unmounted or disabled in qemu.conf), we error out with weird message even though we could continue starting the domain. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1097028 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_cgroup.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 3394c68..0af6ac5 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -949,7 +949,11 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) virCgroupFree(&cgroup_vcpu); } - return -1; + if (period || quota) + return -1; + + virResetLastError(); + return 0; } int @@ -1016,7 +1020,11 @@ qemuSetupCgroupForEmulator(virQEMUDriverPtr driver, virCgroupFree(&cgroup_emulator); } - return -1; + if (period || quota) + return -1; + + virResetLastError(); + return 0; } int -- 2.0.0
On 07/09/2014 10:15 AM, Martin Kletzander wrote:
When creating cgroups for vcpu and emulator threads whilst starting a domain, we explicitly skip creating those cgroups in case priv->cgroup is NULL (cgroups not supported) because SetAffinity() serves the same purpose. If the host supports only some cgroups (the ones we need are either unmounted or disabled in qemu.conf), we error out with weird message even though we could continue starting the domain.
Yet this patch does not change the error message.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1097028
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_cgroup.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 3394c68..0af6ac5 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -949,7 +949,11 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) virCgroupFree(&cgroup_vcpu); }
- return -1; + if (period || quota) + return -1; + + virResetLastError(); + return 0; }
This also resets OOM errors and errors that happen when these controllers are mounted. Can't we just check upfront if the needed controllers are available? Jan
On Wed, Jul 09, 2014 at 01:07:47PM +0200, Ján Tomko wrote:
On 07/09/2014 10:15 AM, Martin Kletzander wrote:
When creating cgroups for vcpu and emulator threads whilst starting a domain, we explicitly skip creating those cgroups in case priv->cgroup is NULL (cgroups not supported) because SetAffinity() serves the same purpose. If the host supports only some cgroups (the ones we need are either unmounted or disabled in qemu.conf), we error out with weird message even though we could continue starting the domain.
Yet this patch does not change the error message.
Yes, because there should be no error.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1097028
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_cgroup.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 3394c68..0af6ac5 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -949,7 +949,11 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) virCgroupFree(&cgroup_vcpu); }
- return -1; + if (period || quota) + return -1; + + virResetLastError(); + return 0; }
This also resets OOM errors and errors that happen when these controllers are mounted.
Can't we just check upfront if the needed controllers are available?
There might be more problems than the controllers not being mounted, but OK, the others are corner cases anyway. Would the following diff be more suitable? diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 3394c68..d4c969b 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -893,6 +893,15 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) return -1; } + /* + * If CPU cgroup controller is not initialized here, than we need + * neither period nor quota settings. And if CPUSET controller is + * not initialized either, than there's nothing to do anyway. + */ + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && + !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) + return 0; + /* We are trying to setup cgroups for CPU pinning, which can also be done * with virProcessSetAffinity, thus the lack of cgroups is not fatal here. */ @@ -972,6 +981,15 @@ qemuSetupCgroupForEmulator(virQEMUDriverPtr driver, return -1; } + /* + * If CPU cgroup controller is not initialized here, than we need + * neither period nor quota settings. And if CPUSET controller is + * not initialized either, than there's nothing to do anyway. + */ + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && + !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) + return 0; + if (priv->cgroup == NULL) return 0; /* Not supported, so claim success */ -- Martin
On 07/09/2014 02:30 PM, Martin Kletzander wrote:
On Wed, Jul 09, 2014 at 01:07:47PM +0200, Ján Tomko wrote:
On 07/09/2014 10:15 AM, Martin Kletzander wrote:
When creating cgroups for vcpu and emulator threads whilst starting a domain, we explicitly skip creating those cgroups in case priv->cgroup is NULL (cgroups not supported) because SetAffinity() serves the same purpose. If the host supports only some cgroups (the ones we need are either unmounted or disabled in qemu.conf), we error out with weird message even though we could continue starting the domain.
Yet this patch does not change the error message.
Yes, because there should be no error.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1097028
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_cgroup.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 3394c68..0af6ac5 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -949,7 +949,11 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) virCgroupFree(&cgroup_vcpu); }
- return -1; + if (period || quota) + return -1; + + virResetLastError(); + return 0; }
This also resets OOM errors and errors that happen when these controllers are mounted.
Can't we just check upfront if the needed controllers are available?
There might be more problems than the controllers not being mounted, but OK, the others are corner cases anyway. Would the following diff be more suitable?
Yes, that is much nicer.
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 3394c68..d4c969b 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -893,6 +893,15 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) return -1; }
+ /* + * If CPU cgroup controller is not initialized here, than we need + * neither period nor quota settings. And if CPUSET controller is + * not initialized either, than there's nothing to do anyway. + */ + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && + !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) + return 0; + /* We are trying to setup cgroups for CPU pinning, which can also be done * with virProcessSetAffinity, thus the lack of cgroups is not fatal here. */ @@ -972,6 +981,15 @@ qemuSetupCgroupForEmulator(virQEMUDriverPtr driver, return -1; }
+ /* + * If CPU cgroup controller is not initialized here, than we need + * neither period nor quota settings. And if CPUSET controller is + * not initialized either, than there's nothing to do anyway. + */ + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && + !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) + return 0; + if (priv->cgroup == NULL) return 0; /* Not supported, so claim success */
s/than/then/ in the comments. ACK with that fixed. Jan
On 07/09/2014 02:15 AM, Martin Kletzander wrote:
When creating cgroups for vcpu and emulator threads whilst starting a domain, we explicitly skip creating those cgroups in case priv->cgroup is NULL (cgroups not supported) because SetAffinity() serves the same purpose. If the host supports only some cgroups (the ones we need are either unmounted or disabled in qemu.conf), we error out with weird message even though we could continue starting the domain.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1097028
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_cgroup.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 3394c68..0af6ac5 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -949,7 +949,11 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) virCgroupFree(&cgroup_vcpu); }
- return -1; + if (period || quota) + return -1; + + virResetLastError(); + return 0;
This still leaks the error message to the log, even if we proceed with execution. Checking up front is better than clearing errors after the fact, if that is possible. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Ján Tomko -
Martin Kletzander