[libvirt] [PATCH v3 0/7] cgroups v2 fixes and improvements
Pavel Hrdina (7): 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 util: vircgroupv2: mark only requested controllers as available src/util/vircgroup.c | 32 +++++++++--------- src/util/vircgroupbackend.h | 3 +- src/util/vircgroupv1.c | 4 ++- src/util/vircgroupv2.c | 67 +++++++++++++++++++++++++++---------- 4 files changed, 71 insertions(+), 35 deletions(-) -- 2.21.0
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@redhat.com> Reviewed-by: Ján Tomko <jtomko@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
This affects only cgroups v2 where enabled controllers are not based on available mount points but on the list provided in cgroup.controllers file. However, moving it will fill in placement as well, so it needs to be freed together with mount point if we don't need that controller. 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@redhat.com> --- Notes: Changes in v3: - Moving the detection after placement means we need to free it together with mount point if that controller is not required. src/util/vircgroup.c | 32 ++++++++++++++++---------------- src/util/vircgroupv1.c | 1 + src/util/vircgroupv2.c | 5 +++-- 3 files changed, 20 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/vircgroupv1.c b/src/util/vircgroupv1.c index fb3e0b2d47..4231d8d6fa 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -464,6 +464,7 @@ virCgroupV1DetectControllers(virCgroupPtr group, } } VIR_FREE(group->legacy[i].mountPoint); + VIR_FREE(group->legacy[i].placement); } } } else { 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
On Tue, Jun 25, 2019 at 13:16:21 +0200, Pavel Hrdina wrote:
This affects only cgroups v2 where enabled controllers are not based on available mount points but on the list provided in cgroup.controllers file. However, moving it will fill in placement as well, so it needs to be freed together with mount point if we don't need that controller.
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@redhat.com> ---
Notes: Changes in v3: - Moving the detection after placement means we need to free it together with mount point if that controller is not required.
ACK
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@redhat.com> Reviewed-by: Ján Tomko <jtomko@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
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@redhat.com> Reviewed-by: Ján Tomko <jtomko@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
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@redhat.com> --- Notes: 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
On Tue, Jun 25, 2019 at 13:16:24 +0200, Pavel Hrdina wrote:
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@redhat.com> ---
Notes: Introduced in v2
ACK although based on the review of the next patch you probably also want to add a boolean to suppres the error reporting in case of a known ignorable error.
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@redhat.com> --- 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
On Tue, Jun 25, 2019 at 13:16:25 +0200, Pavel Hrdina wrote:
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@redhat.com> --- 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();
Instead of doing this you should not report the error in the first place. Given that the refactor in the previous commit adds the error report in the called function it should be trivial to do so. Without that the logs would be spammed by an error which does not help the users much.
On Wed, Jun 26, 2019 at 09:21:59AM +0200, Peter Krempa wrote:
On Tue, Jun 25, 2019 at 13:16:25 +0200, Pavel Hrdina wrote:
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@redhat.com> --- 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();
Instead of doing this you should not report the error in the first place. Given that the refactor in the previous commit adds the error report in the called function it should be trivial to do so.
Without that the logs would be spammed by an error which does not help the users much.
Works for me, this was based on our in person discussion but I guess there was some misunderstanding. Do you need v4? Pavel
On Wed, Jun 26, 2019 at 10:23:25 +0200, Pavel Hrdina wrote:
On Wed, Jun 26, 2019 at 09:21:59AM +0200, Peter Krempa wrote:
On Tue, Jun 25, 2019 at 13:16:25 +0200, Pavel Hrdina wrote:
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@redhat.com> --- 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();
Instead of doing this you should not report the error in the first place. Given that the refactor in the previous commit adds the error report in the called function it should be trivial to do so.
Without that the logs would be spammed by an error which does not help the users much.
Works for me, this was based on our in person discussion but I guess there was some misunderstanding. Do you need v4?
No, that's not necessary. You can consider that ACK'd
On Wed, Jun 26, 2019 at 10:27:31AM +0200, Peter Krempa wrote:
On Wed, Jun 26, 2019 at 10:23:25 +0200, Pavel Hrdina wrote:
On Wed, Jun 26, 2019 at 09:21:59AM +0200, Peter Krempa wrote:
On Tue, Jun 25, 2019 at 13:16:25 +0200, Pavel Hrdina wrote:
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@redhat.com> --- 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();
Instead of doing this you should not report the error in the first place. Given that the refactor in the previous commit adds the error report in the called function it should be trivial to do so.
Without that the logs would be spammed by an error which does not help the users much.
Works for me, this was based on our in person discussion but I guess there was some misunderstanding. Do you need v4?
No, that's not necessary. You can consider that ACK'd
Thanks, all the issues are fixed and pushed now. Pavel
When detecting available controllers on host we can be limited by list of controllers from qemu.conf file. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- Notes: Introduced in v3 src/util/vircgroupv2.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 348c12d5c6..fd883f3c7f 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -302,15 +302,15 @@ virCgroupV2DetectControllers(virCgroupPtr group, group->unified.controllers |= 1 << VIR_CGROUP_CONTROLLER_CPUACCT; } + if (controllers >= 0) + group->unified.controllers &= controllers; + for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) VIR_DEBUG("Controller '%s' present=%s", virCgroupV2ControllerTypeToString(i), (group->unified.controllers & 1 << i) ? "yes" : "no"); - if (controllers >= 0) - return controllers & group->unified.controllers; - else - return group->unified.controllers; + return group->unified.controllers; } -- 2.21.0
On Tue, Jun 25, 2019 at 13:16:26 +0200, Pavel Hrdina wrote:
When detecting available controllers on host we can be limited by list of controllers from qemu.conf file.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> ---
Notes: Introduced in v3
src/util/vircgroupv2.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 348c12d5c6..fd883f3c7f 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -302,15 +302,15 @@ virCgroupV2DetectControllers(virCgroupPtr group, group->unified.controllers |= 1 << VIR_CGROUP_CONTROLLER_CPUACCT; }
+ if (controllers >= 0) + group->unified.controllers &= controllers;
The use of 'int' here for 'controllers' makes it super non-obvious and super sketchy in what's happening here. Especially since you are then doing bitwise operations with it afterwards. It's pre-existing though. Still super ugly.
+ for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) VIR_DEBUG("Controller '%s' present=%s", virCgroupV2ControllerTypeToString(i), (group->unified.controllers & 1 << i) ? "yes" : "no");
- if (controllers >= 0) - return controllers & group->unified.controllers; - else - return group->unified.controllers; + return group->unified.controllers; }
ACK
participants (2)
-
Pavel Hrdina -
Peter Krempa