[libvirt] [PATCH 0/5] cgroups v2 fixes and improvements
Pavel Hrdina (5): 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: 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 | 36 ++++++++++++++++++++++++------------ 4 files changed, 44 insertions(+), 30 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> --- 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 f58e336404..bb7452d2a0 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 8ce10d3608..3c195f69ce 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 0cfbc96264..671a078b23 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
On Thu, Jun 20, 2019 at 01:25:41PM +0200, Pavel Hrdina wrote:
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> --- 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(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@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 bb7452d2a0..3a69698a8b 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 671a078b23..186c4fc76b 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 Thu, Jun 20, 2019 at 01:25:42PM +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.
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> --- src/util/vircgroup.c | 32 ++++++++++++++++---------------- src/util/vircgroupv2.c | 5 +++-- 2 files changed, 19 insertions(+), 18 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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> --- 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 186c4fc76b..f3a119b689 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
On Thu, Jun 20, 2019 at 01:25:43PM +0200, Pavel Hrdina wrote:
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> --- src/util/vircgroupv2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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> --- 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 f3a119b689..6eeebe2230 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
On Thu, Jun 20, 2019 at 01:25:44PM +0200, Pavel Hrdina wrote:
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> --- src/util/vircgroupv2.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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 | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 6eeebe2230..9cc64d64d2 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -425,8 +425,13 @@ virCgroupV2MakeGroup(virCgroupPtr parent ATTRIBUTE_UNUSED, if (i == VIR_CGROUP_CONTROLLER_CPUACCT) continue; - if (virCgroupV2EnableController(parent, i) < 0) - return -1; + if (virCgroupV2EnableController(parent, i) < 0) { + virResetLastError(); + VIR_DEBUG("failed to enable '%s' controller, skipping", + virCgroupV2ControllerTypeToString(i)); + group->unified.controllers &= ~(1 << i); + continue; + } } } } -- 2.21.0
On Thu, Jun 20, 2019 at 01:25:45PM +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 | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 6eeebe2230..9cc64d64d2 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -425,8 +425,13 @@ virCgroupV2MakeGroup(virCgroupPtr parent ATTRIBUTE_UNUSED, if (i == VIR_CGROUP_CONTROLLER_CPUACCT) continue;
- if (virCgroupV2EnableController(parent, i) < 0) - return -1; + if (virCgroupV2EnableController(parent, i) < 0) { + virResetLastError();
If it's not an error, we should not have logged it in the first place. Jano
+ VIR_DEBUG("failed to enable '%s' controller, skipping", + virCgroupV2ControllerTypeToString(i)); + group->unified.controllers &= ~(1 << i); + continue; + } } } } -- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Jun 21, 2019 at 11:58:16AM +0200, Ján Tomko wrote:
On Thu, Jun 20, 2019 at 01:25:45PM +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 | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 6eeebe2230..9cc64d64d2 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -425,8 +425,13 @@ virCgroupV2MakeGroup(virCgroupPtr parent ATTRIBUTE_UNUSED, if (i == VIR_CGROUP_CONTROLLER_CPUACCT) continue;
- if (virCgroupV2EnableController(parent, i) < 0) - return -1; + if (virCgroupV2EnableController(parent, i) < 0) { + virResetLastError();
If it's not an error, we should not have logged it in the first place.
After discussion in person we will keep the error message logged but I'll rewrite it a little bit to have a better error message. I'll post v2 shortly, thanks for review. Pavel
participants (2)
-
Ján Tomko -
Pavel Hrdina