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@xxxxxxxxxx> >>> --- >>> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list