Re: [libvirt] [PATCH] qemu: don't error out when cgroups don't exist
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(a)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
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)