3:15 a.m.
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(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;
}
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
6:07 a.m.
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(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?
Jan
7:30 a.m.
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
7:45 a.m.
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(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?
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
8:25 a.m.
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(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 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
3784
days inactive
3784
days old
4 comments
3 participants
participants (3)
-
Eric Blake -
Ján Tomko -
Martin Kletzander