8:32 a.m.
Pavel Hrdina (6):
util: vircgroup: pass parent cgroup into virCgroupDetectControllersCB
util: vircgroup: improve controller detection
util: vircgroupv2: use any controller to create thread directory
util: vircgroupv2: enable CPU controller only if it's available
util: vircgroupv2: separate return values of
virCgroupV2EnableController
util: vircgroupv2: don't error out if enabling controller fails
src/util/vircgroup.c | 32 ++++++++++----------
src/util/vircgroupbackend.h | 3 +-
src/util/vircgroupv1.c | 3 +-
src/util/vircgroupv2.c | 59 +++++++++++++++++++++++++++++--------
4 files changed, 66 insertions(+), 31 deletions(-)
--
2.21.0
8:32 a.m.
New subject: [libvirt] [PATCH v2 1/6] util: vircgroup: pass parent cgroup into virCgroupDetectControllersCB
In cgroups v2 we don't have to detect available controllers every single
time if we are creating a new cgroup based on parent cgroup.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
---
src/util/vircgroup.c | 2 +-
src/util/vircgroupbackend.h | 3 ++-
src/util/vircgroupv1.c | 3 ++-
src/util/vircgroupv2.c | 17 +++++++++++------
4 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index e32215935b..b7e5f03521 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -383,7 +383,7 @@ virCgroupDetect(virCgroupPtr group,
for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
if (group->backends[i]) {
- int rc = group->backends[i]->detectControllers(group, controllers);
+ int rc = group->backends[i]->detectControllers(group, controllers,
parent);
if (rc < 0)
return -1;
controllersAvailable |= rc;
diff --git a/src/util/vircgroupbackend.h b/src/util/vircgroupbackend.h
index e58e327c68..1fe0851184 100644
--- a/src/util/vircgroupbackend.h
+++ b/src/util/vircgroupbackend.h
@@ -95,7 +95,8 @@ typedef char *
typedef int
(*virCgroupDetectControllersCB)(virCgroupPtr group,
- int controllers);
+ int controllers,
+ virCgroupPtr parent);
typedef bool
(*virCgroupHasControllerCB)(virCgroupPtr cgroup,
diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 97258917bc..fb3e0b2d47 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -420,7 +420,8 @@ virCgroupV1StealPlacement(virCgroupPtr group)
static int
virCgroupV1DetectControllers(virCgroupPtr group,
- int controllers)
+ int controllers,
+ virCgroupPtr parent ATTRIBUTE_UNUSED)
{
size_t i;
size_t j;
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index 1179c4459a..2d09d77a29 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -285,16 +285,21 @@ virCgroupV2ParseControllersFile(virCgroupPtr group)
static int
virCgroupV2DetectControllers(virCgroupPtr group,
- int controllers)
+ int controllers,
+ virCgroupPtr parent)
{
size_t i;
- if (virCgroupV2ParseControllersFile(group) < 0)
- return -1;
+ if (parent) {
+ group->unified.controllers = parent->unified.controllers;
+ } else {
+ if (virCgroupV2ParseControllersFile(group) < 0)
+ return -1;
- /* In cgroup v2 there is no cpuacct controller, the cpu.stat file always
- * exists with usage stats. */
- group->unified.controllers |= 1 << VIR_CGROUP_CONTROLLER_CPUACCT;
+ /* In cgroup v2 there is no cpuacct controller, the cpu.stat file always
+ * exists with usage stats. */
+ group->unified.controllers |= 1 << VIR_CGROUP_CONTROLLER_CPUACCT;
+ }
for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++)
VIR_DEBUG("Controller '%s' present=%s",
--
2.21.0
8:32 a.m.
New subject: [libvirt] [PATCH v2 2/6] util: vircgroup: improve controller detection
This affects only cgroups v2 where enabled controllers are not based on
available mount points but on the list provided in cgroup.controllers
file.
Before this patch we were assuming that all controllers available in
root cgroup where available in all other sub-cgroups which was wrong.
In order to fix it we need to move the cgroup controllers detection
after cgroup placement was prepared in order to build correct path for
cgroup.controllers file.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
---
src/util/vircgroup.c | 32 ++++++++++++++++----------------
src/util/vircgroupv2.c | 5 +++--
2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index b7e5f03521..da506fc0b0 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -381,22 +381,6 @@ virCgroupDetect(virCgroupPtr group,
return -1;
}
- for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
- if (group->backends[i]) {
- int rc = group->backends[i]->detectControllers(group, controllers,
parent);
- if (rc < 0)
- return -1;
- controllersAvailable |= rc;
- }
- }
-
- /* Check that at least 1 controller is available */
- if (controllersAvailable == 0) {
- virReportSystemError(ENXIO, "%s",
- _("At least one cgroup controller is required"));
- return -1;
- }
-
/* In some cases we can copy part of the placement info
* based on the parent cgroup...
*/
@@ -421,6 +405,22 @@ virCgroupDetect(virCgroupPtr group,
}
}
+ for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
+ if (group->backends[i]) {
+ int rc = group->backends[i]->detectControllers(group, controllers,
parent);
+ if (rc < 0)
+ return -1;
+ controllersAvailable |= rc;
+ }
+ }
+
+ /* Check that at least 1 controller is available */
+ if (controllersAvailable == 0) {
+ virReportSystemError(ENXIO, "%s",
+ _("At least one cgroup controller is required"));
+ return -1;
+ }
+
return 0;
}
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index 2d09d77a29..29b5806a01 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -250,8 +250,9 @@ virCgroupV2ParseControllersFile(virCgroupPtr group)
char **contList = NULL;
char **tmp;
- if (virAsprintf(&contFile, "%s/cgroup.controllers",
- group->unified.mountPoint) < 0)
+ if (virAsprintf(&contFile, "%s%s/cgroup.controllers",
+ group->unified.mountPoint,
+ NULLSTR_EMPTY(group->unified.placement)) < 0)
return -1;
rc = virFileReadAll(contFile, 1024 * 1024, &contStr);
--
2.21.0
8:32 a.m.
New subject: [libvirt] [PATCH v2 3/6] util: vircgroupv2: use any controller to create thread directory
The assumption that CPU controller would be always enabled is wrong, we
should use any available controller to create a new sub-cgroup.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
---
src/util/vircgroupv2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index 29b5806a01..6bcbb7e1a0 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -399,7 +399,7 @@ virCgroupV2MakeGroup(virCgroupPtr parent ATTRIBUTE_UNUSED,
if (create) {
if (flags & VIR_CGROUP_THREAD) {
- if (virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_CPU,
+ if (virCgroupSetValueStr(group, controller,
"cgroup.type", "threaded") <
0) {
return -1;
}
--
2.21.0
8:32 a.m.
New subject: [libvirt] [PATCH v2 4/6] util: vircgroupv2: enable CPU controller only if it's available
It might happen that we are not able to enable CPU controller so we
can enable it for thread sub-cgroups only if it's available in parent
cgroup.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
---
src/util/vircgroupv2.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index 6bcbb7e1a0..3f4548b532 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -404,7 +404,8 @@ virCgroupV2MakeGroup(virCgroupPtr parent ATTRIBUTE_UNUSED,
return -1;
}
- if (virCgroupV2EnableController(parent,
+ if (virCgroupV2HasController(parent, VIR_CGROUP_CONTROLLER_CPU) &&
+ virCgroupV2EnableController(parent,
VIR_CGROUP_CONTROLLER_CPU) < 0) {
return -1;
}
--
2.21.0
8:32 a.m.
New subject: [libvirt] [PATCH v2 5/6] util: vircgroupv2: separate return values of virCgroupV2EnableController
In order to skip controllers that we are not able to activate we need
to return different return value so the caller can decide what to do.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
Introduced in v2.
src/util/vircgroupv2.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index 3f4548b532..133a8e0e66 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -353,22 +353,37 @@ virCgroupV2PathOfController(virCgroupPtr group,
}
+/**
+ * virCgroupV2EnableController:
+ *
+ * Returns: -1 on fatal error
+ * -2 if we failed to write into cgroup.subtree_control
+ * 0 on success
+ */
static int
virCgroupV2EnableController(virCgroupPtr parent,
int controller)
{
VIR_AUTOFREE(char *) val = NULL;
+ VIR_AUTOFREE(char *) path = NULL;
if (virAsprintf(&val, "+%s",
virCgroupV2ControllerTypeToString(controller)) < 0) {
return -1;
}
- if (virCgroupSetValueStr(parent, controller,
- "cgroup.subtree_control", val) < 0) {
+ if (virCgroupPathOfController(parent, controller,
+ "cgroup.subtree_control", &path) < 0)
{
return -1;
}
+ if (virFileWriteStr(path, val, 0) < 0) {
+ virReportSystemError(errno,
+ _("Failed to enable controller '%s' for
'%s'"),
+ val, path);
+ return -2;
+ }
+
return 0;
}
--
2.21.0
8:32 a.m.
New subject: [libvirt] [PATCH v2 6/6] util: vircgroupv2: don't error out if enabling controller fails
Currently CPU controller cannot be enabled if there is any real-time
task running and is assigned to non-root cgroup which is the case on
several distributions with graphical environment.
Instead of erroring out treat it as the controller is not available.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
Changes in v2:
- Reworked error reporting to virResetLastError only if we fail
to enable controller. The error message will be still logged
but now it's more descriptive what have failed.
src/util/vircgroupv2.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index 133a8e0e66..348c12d5c6 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -433,6 +433,8 @@ virCgroupV2MakeGroup(virCgroupPtr parent ATTRIBUTE_UNUSED,
} else {
size_t i;
for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
+ int rc;
+
if (!virCgroupV2HasController(parent, i))
continue;
@@ -440,8 +442,17 @@ virCgroupV2MakeGroup(virCgroupPtr parent ATTRIBUTE_UNUSED,
if (i == VIR_CGROUP_CONTROLLER_CPUACCT)
continue;
- if (virCgroupV2EnableController(parent, i) < 0)
+ rc = virCgroupV2EnableController(parent, i);
+ if (rc < 0) {
+ if (rc == -2) {
+ virResetLastError();
+ VIR_DEBUG("failed to enable '%s' controller,
skipping",
+ virCgroupV2ControllerTypeToString(i));
+ group->unified.controllers &= ~(1 << i);
+ continue;
+ }
return -1;
+ }
}
}
}
--
2.21.0
5:41 a.m.
On Fri, Jun 21, 2019 at 03:32:40PM +0200, Pavel Hrdina wrote:
Pavel Hrdina (6):
util: vircgroup: pass parent cgroup into virCgroupDetectControllersCB
util: vircgroup: improve controller detection
util: vircgroupv2: use any controller to create thread directory
util: vircgroupv2: enable CPU controller only if it's available
util: vircgroupv2: separate return values of
virCgroupV2EnableController
util: vircgroupv2: don't error out if enabling controller fails
SNACK, there are some other issues that needs to be fixed and patch 2
was not complete. I'll post v3 shortly.
Pavel
1946
days inactive
1950
days old
7 comments
1 participants
participants (1)
-
Pavel Hrdina