When creating cgroup hierarchy we need to enable controllers in the
parent cgroup in order to be usable. That means writing "+{controller}"
into cgroup.subtree_control file. We can enable only controllers that
are enabled for parent cgroup, that means we need to do that for the
whole cgroup tree.
Cgroups for threads needs to be handled differently in cgroup v2. There
are two types of controllers:
- domain controllers: these cannot be enabled for threads
- threaded controllers: these can be enabled for threads
In addition there are multiple types of cgroups:
- domain: normal cgroup
- domain threaded: a domain cgroup that serves as root for threaded
cgroups
- domain invalid: invalid cgroup, can be changed into threaded, this
is the default state if you create subgroup inside
domain threaded group or threaded group
- threaded: threaded cgroup which can have domain threaded or
threaded as parent group
In order to create threaded cgroup it's sufficient to write "threaded"
into cgroup.type file, it will automatically make parent cgroup
"domain threaded" if it was only "domain". In case the parent cgroup
is already "domain threaded" or "threaded" it will modify only the type
of current cgroup. After that we can enable threaded controllers.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
src/util/vircgroup.c | 2 +-
src/util/vircgroupbackend.h | 1 +
src/util/vircgroupv2.c | 78 +++++++++++++++++++++++++++++++++++++
3 files changed, 80 insertions(+), 1 deletion(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 1097b1f998..dc249bfe33 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -938,7 +938,7 @@ virCgroupNewThread(virCgroupPtr domain,
if (virCgroupNew(-1, name, domain, controllers, group) < 0)
return -1;
- if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_NONE) < 0) {
+ if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_THREAD) < 0) {
virCgroupFree(group);
return -1;
}
diff --git a/src/util/vircgroupbackend.h b/src/util/vircgroupbackend.h
index b1f19233e4..86d1539e07 100644
--- a/src/util/vircgroupbackend.h
+++ b/src/util/vircgroupbackend.h
@@ -33,6 +33,7 @@ typedef enum {
* before creating subcgroups and
* attaching tasks
*/
+ VIR_CGROUP_THREAD = 1 << 1, /* cgroup v2 handles threads differently */
} virCgroupBackendFlags;
typedef enum {
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index 3ca463e4c2..8fb9ace474 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -336,6 +336,83 @@ virCgroupV2PathOfController(virCgroupPtr group,
}
+static int
+virCgroupV2EnableController(virCgroupPtr parent,
+ int controller)
+{
+ VIR_AUTOFREE(char *) val = NULL;
+
+ if (virAsprintf(&val, "+%s",
+ virCgroupV2ControllerTypeToString(controller)) < 0) {
+ return -1;
+ }
+
+ if (virCgroupSetValueStr(parent, controller,
+ "cgroup.subtree_control", val) < 0) {
+ return -1;
+ }
+
+ return 0;
+}
+
+
+static int
+virCgroupV2MakeGroup(virCgroupPtr parent ATTRIBUTE_UNUSED,
+ virCgroupPtr group,
+ bool create,
+ unsigned int flags)
+{
+ VIR_AUTOFREE(char *) path = NULL;
+ int controller;
+
+ VIR_DEBUG("Make group %s", group->path);
+
+ controller = virCgroupV2GetAnyController(group);
+ if (virCgroupV2PathOfController(group, controller, "", &path) < 0)
+ return -1;
+
+ VIR_DEBUG("Make controller %s", path);
+
+ if (!virFileExists(path)) {
+ if (!create || mkdir(path, 0755 || errno == EEXIST) < 0) {
+ virReportSystemError(errno,
+ _("Failed to create v2 cgroup '%s'"),
+ group->path);
+ return -1;
+ }
+ }
+
+ if (create) {
+ if (flags & VIR_CGROUP_THREAD) {
+ if (virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_CPU,
+ "cgroup.type", "threaded") < 0) {
+ return -1;
+ }
+
+ if (virCgroupV2EnableController(parent,
+ VIR_CGROUP_CONTROLLER_CPU) < 0) {
+ return -1;
+ }
+ } else {
+ size_t i;
+ for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
+ if (!virCgroupV2HasController(parent, i))
+ continue;
+
+ /* Controllers that are implicitly enabled if available. */
+ if (i == VIR_CGROUP_CONTROLLER_CPUACCT)
+ continue;
+
+ if (virCgroupV2EnableController(parent, i) < 0)
+ return -1;
+ }
+ }
+ }
+
+ return 0;
+}
+
+
virCgroupBackend virCgroupV2Backend = {
.type = VIR_CGROUP_BACKEND_TYPE_V2,
@@ -351,6 +428,7 @@ virCgroupBackend virCgroupV2Backend = {
.hasController = virCgroupV2HasController,
.getAnyController = virCgroupV2GetAnyController,
.pathOfController = virCgroupV2PathOfController,
+ .makeGroup = virCgroupV2MakeGroup,
};
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2018-10-02 at 10:43 +0200, Pavel Hrdina wrote: > When creating cgroup hierarchy we need to enable controllers in the > parent cgroup in order to be usable. That means writing > "+{controller}" > into cgroup.subtree_control file. We can enable only controllers > that > are enabled for parent cgroup, that means we need to do that for the > whole cgroup tree. > > Cgroups for threads needs to be handled differently in cgroup > v2. There > are two types of controllers: > > - domain controllers: these cannot be enabled for threads > - threaded controllers: these can be enabled for threads > > In addition there are multiple types of cgroups: > > - domain: normal cgroup > - domain threaded: a domain cgroup that serves as root for > threaded > cgroups > - domain invalid: invalid cgroup, can be changed into threaded, > this > is the default state if you create subgroup > inside > domain threaded group or threaded group > - threaded: threaded cgroup which can have domain threaded or > threaded as parent group > > In order to create threaded cgroup it's sufficient to write > "threaded" > into cgroup.type file, it will automatically make parent cgroup > "domain threaded" if it was only "domain". In case the parent cgroup > is already "domain threaded" or "threaded" it will modify only the > type > of current cgroup. After that we can enable threaded controllers. > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > src/util/vircgroup.c | 2 +- > src/util/vircgroupbackend.h | 1 + > src/util/vircgroupv2.c | 78 > +++++++++++++++++++++++++++++++++++++ > 3 files changed, 80 insertions(+), 1 deletion(-) > > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > index 1097b1f998..dc249bfe33 100644 > --- a/src/util/vircgroup.c > +++ b/src/util/vircgroup.c > @@ -938,7 +938,7 @@ virCgroupNewThread(virCgroupPtr domain, > if (virCgroupNew(-1, name, domain, controllers, group) < 0) > return -1; > > - if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_NONE) > < 0) { > + if (virCgroupMakeGroup(domain, *group, create, > VIR_CGROUP_THREAD) < 0) { > virCgroupFree(group); > return -1; > } > diff --git a/src/util/vircgroupbackend.h > b/src/util/vircgroupbackend.h > index b1f19233e4..86d1539e07 100644 > --- a/src/util/vircgroupbackend.h > +++ b/src/util/vircgroupbackend.h > @@ -33,6 +33,7 @@ typedef enum { > * before creating subcgroups > and > * attaching tasks > */ > + VIR_CGROUP_THREAD = 1 << 1, /* cgroup v2 handles threads > differently */ > } virCgroupBackendFlags; > > typedef enum { > diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c > index 3ca463e4c2..8fb9ace474 100644 > --- a/src/util/vircgroupv2.c > +++ b/src/util/vircgroupv2.c > @@ -336,6 +336,83 @@ virCgroupV2PathOfController(virCgroupPtr group, > } > > > +static int > +virCgroupV2EnableController(virCgroupPtr parent, > + int controller) > +{ > + VIR_AUTOFREE(char *) val = NULL; > + > + if (virAsprintf(&val, "+%s", > + virCgroupV2ControllerTypeToString(controller)) < > 0) { > + return -1; > + } > + > + if (virCgroupSetValueStr(parent, controller, > + "cgroup.subtree_control", val) < 0) { > + return -1; > + } > + > + return 0; > +} > + > + > +static int > +virCgroupV2MakeGroup(virCgroupPtr parent ATTRIBUTE_UNUSED, > + virCgroupPtr group, > + bool create, > + unsigned int flags) > +{ > + VIR_AUTOFREE(char *) path = NULL; > + int controller; > + > + VIR_DEBUG("Make group %s", group->path); > + > + controller = virCgroupV2GetAnyController(group); > + if (virCgroupV2PathOfController(group, controller, "", &path) < > 0) > + return -1; > + > + VIR_DEBUG("Make controller %s", path); > + > + if (!virFileExists(path)) { > + if (!create || mkdir(path, 0755 || errno == EEXIST) < 0) { The `mdkir(path, 0755 || errno == EEXIST) < 0` part of the check doesn't look right. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Oct 04, 2018 at 02:43:18PM +0200, Fabiano Fidêncio wrote: > On Tue, 2018-10-02 at 10:43 +0200, Pavel Hrdina wrote: > > When creating cgroup hierarchy we need to enable controllers in the > > parent cgroup in order to be usable. That means writing > > "+{controller}" > > into cgroup.subtree_control file. We can enable only controllers > > that > > are enabled for parent cgroup, that means we need to do that for the > > whole cgroup tree. > > > > Cgroups for threads needs to be handled differently in cgroup > > v2. There > > are two types of controllers: > > > > - domain controllers: these cannot be enabled for threads > > - threaded controllers: these can be enabled for threads > > > > In addition there are multiple types of cgroups: > > > > - domain: normal cgroup > > - domain threaded: a domain cgroup that serves as root for > > threaded > > cgroups > > - domain invalid: invalid cgroup, can be changed into threaded, > > this > > is the default state if you create subgroup > > inside > > domain threaded group or threaded group > > - threaded: threaded cgroup which can have domain threaded or > > threaded as parent group > > > > In order to create threaded cgroup it's sufficient to write > > "threaded" > > into cgroup.type file, it will automatically make parent cgroup > > "domain threaded" if it was only "domain". In case the parent cgroup > > is already "domain threaded" or "threaded" it will modify only the > > type > > of current cgroup. After that we can enable threaded controllers. > > > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > > --- > > src/util/vircgroup.c | 2 +- > > src/util/vircgroupbackend.h | 1 + > > src/util/vircgroupv2.c | 78 > > +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 80 insertions(+), 1 deletion(-) > > > > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > > index 1097b1f998..dc249bfe33 100644 > > --- a/src/util/vircgroup.c > > +++ b/src/util/vircgroup.c > > @@ -938,7 +938,7 @@ virCgroupNewThread(virCgroupPtr domain, > > if (virCgroupNew(-1, name, domain, controllers, group) < 0) > > return -1; > > > > - if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_NONE) > > < 0) { > > + if (virCgroupMakeGroup(domain, *group, create, > > VIR_CGROUP_THREAD) < 0) { > > virCgroupFree(group); > > return -1; > > } > > diff --git a/src/util/vircgroupbackend.h > > b/src/util/vircgroupbackend.h > > index b1f19233e4..86d1539e07 100644 > > --- a/src/util/vircgroupbackend.h > > +++ b/src/util/vircgroupbackend.h > > @@ -33,6 +33,7 @@ typedef enum { > > * before creating subcgroups > > and > > * attaching tasks > > */ > > + VIR_CGROUP_THREAD = 1 << 1, /* cgroup v2 handles threads > > differently */ > > } virCgroupBackendFlags; > > > > typedef enum { > > diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c > > index 3ca463e4c2..8fb9ace474 100644 > > --- a/src/util/vircgroupv2.c > > +++ b/src/util/vircgroupv2.c > > @@ -336,6 +336,83 @@ virCgroupV2PathOfController(virCgroupPtr group, > > } > > > > > > +static int > > +virCgroupV2EnableController(virCgroupPtr parent, > > + int controller) > > +{ > > + VIR_AUTOFREE(char *) val = NULL; > > + > > + if (virAsprintf(&val, "+%s", > > + virCgroupV2ControllerTypeToString(controller)) < > > 0) { > > + return -1; > > + } > > + > > + if (virCgroupSetValueStr(parent, controller, > > + "cgroup.subtree_control", val) < 0) { > > + return -1; > > + } > > + > > + return 0; > > +} > > + > > + > > +static int > > +virCgroupV2MakeGroup(virCgroupPtr parent ATTRIBUTE_UNUSED, > > + virCgroupPtr group, > > + bool create, > > + unsigned int flags) > > +{ > > + VIR_AUTOFREE(char *) path = NULL; > > + int controller; > > + > > + VIR_DEBUG("Make group %s", group->path); > > + > > + controller = virCgroupV2GetAnyController(group); > > + if (virCgroupV2PathOfController(group, controller, "", &path) < > > 0) > > + return -1; > > + > > + VIR_DEBUG("Make controller %s", path); > > + > > + if (!virFileExists(path)) { > > + if (!create || mkdir(path, 0755 || errno == EEXIST) < 0) { > > The `mdkir(path, 0755 || errno == EEXIST) < 0` part of the check > doesn't look right. Nice catch, I'll fix that, thanks. Pavel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 10/02/2018 10:43 AM, Pavel Hrdina wrote: > When creating cgroup hierarchy we need to enable controllers in the > parent cgroup in order to be usable. That means writing "+{controller}" > into cgroup.subtree_control file. We can enable only controllers that > are enabled for parent cgroup, that means we need to do that for the > whole cgroup tree. > > Cgroups for threads needs to be handled differently in cgroup v2. There > are two types of controllers: > > - domain controllers: these cannot be enabled for threads > - threaded controllers: these can be enabled for threads > > In addition there are multiple types of cgroups: > > - domain: normal cgroup > - domain threaded: a domain cgroup that serves as root for threaded > cgroups > - domain invalid: invalid cgroup, can be changed into threaded, this > is the default state if you create subgroup inside > domain threaded group or threaded group > - threaded: threaded cgroup which can have domain threaded or > threaded as parent group > > In order to create threaded cgroup it's sufficient to write "threaded" > into cgroup.type file, it will automatically make parent cgroup > "domain threaded" if it was only "domain". In case the parent cgroup > is already "domain threaded" or "threaded" it will modify only the type > of current cgroup. After that we can enable threaded controllers. > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > src/util/vircgroup.c | 2 +- > src/util/vircgroupbackend.h | 1 + > src/util/vircgroupv2.c | 78 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 80 insertions(+), 1 deletion(-) > > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > index 1097b1f998..dc249bfe33 100644 > --- a/src/util/vircgroup.c > +++ b/src/util/vircgroup.c > @@ -938,7 +938,7 @@ virCgroupNewThread(virCgroupPtr domain, > if (virCgroupNew(-1, name, domain, controllers, group) < 0) > return -1; > > - if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_NONE) < 0) { > + if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_THREAD) < 0) { So unsupported flags are ignored? > virCgroupFree(group); > return -1; > } > diff --git a/src/util/vircgroupbackend.h b/src/util/vircgroupbackend.h > index b1f19233e4..86d1539e07 100644 > --- a/src/util/vircgroupbackend.h > +++ b/src/util/vircgroupbackend.h > @@ -33,6 +33,7 @@ typedef enum { > * before creating subcgroups and > * attaching tasks > */ > + VIR_CGROUP_THREAD = 1 << 1, /* cgroup v2 handles threads differently */ > } virCgroupBackendFlags; > > typedef enum { > diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c > index 3ca463e4c2..8fb9ace474 100644 > --- a/src/util/vircgroupv2.c > +++ b/src/util/vircgroupv2.c > @@ -336,6 +336,83 @@ virCgroupV2PathOfController(virCgroupPtr group, > } > > > +static int > +virCgroupV2EnableController(virCgroupPtr parent, > + int controller) > +{ > + VIR_AUTOFREE(char *) val = NULL; > + > + if (virAsprintf(&val, "+%s", > + virCgroupV2ControllerTypeToString(controller)) < 0) { > + return -1; > + } > + > + if (virCgroupSetValueStr(parent, controller, > + "cgroup.subtree_control", val) < 0) { > + return -1; > + } > + > + return 0; > +} > + > + > +static int > +virCgroupV2MakeGroup(virCgroupPtr parent ATTRIBUTE_UNUSED, > + virCgroupPtr group, > + bool create, > + unsigned int flags) > +{ > + VIR_AUTOFREE(char *) path = NULL; > + int controller; > + > + VIR_DEBUG("Make group %s", group->path); > + > + controller = virCgroupV2GetAnyController(group); > + if (virCgroupV2PathOfController(group, controller, "", &path) < 0) > + return -1; > + > + VIR_DEBUG("Make controller %s", path); > + > + if (!virFileExists(path)) { This should have been virFileIsDir() because EEXIST doesn't mean the @path is a directory. It could be a regular file. > + if (!create || mkdir(path, 0755 || errno == EEXIST) < 0) { Ooops, misplaced ')'. mkdir() || errno ;-) This is why I tend to write each condition on a separate line. But if you want to ignore EEXIST then you need to rewrite this check as follows: if (!create || (mkdir() < 0 && errno != EEXIST)) However, I don't think you want to ignore any errno. Also, any reason these two if()-s should not be merged into one? > + virReportSystemError(errno, > + _("Failed to create v2 cgroup '%s'"), > + group->path); > + return -1; > + } > + } > + > + if (create) { > + if (flags & VIR_CGROUP_THREAD) { > + if (virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_CPU, > + "cgroup.type", "threaded") < 0) { > + return -1; > + } > + > + if (virCgroupV2EnableController(parent, > + VIR_CGROUP_CONTROLLER_CPU) < 0) { > + return -1; > + } > + } else { > + size_t i; > + for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { > + if (!virCgroupV2HasController(parent, i)) > + continue; > + > + /* Controllers that are implicitly enabled if available. */ > + if (i == VIR_CGROUP_CONTROLLER_CPUACCT) > + continue; > + > + if (virCgroupV2EnableController(parent, i) < 0) > + return -1; > + } > + } > + } > + > + return 0; > +} > + > + > virCgroupBackend virCgroupV2Backend = { > .type = VIR_CGROUP_BACKEND_TYPE_V2, > > @@ -351,6 +428,7 @@ virCgroupBackend virCgroupV2Backend = { > .hasController = virCgroupV2HasController, > .getAnyController = virCgroupV2GetAnyController, > .pathOfController = virCgroupV2PathOfController, > + .makeGroup = virCgroupV2MakeGroup, > }; > > > Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Oct 04, 2018 at 02:55:25PM +0200, Michal Privoznik wrote: > On 10/02/2018 10:43 AM, Pavel Hrdina wrote: > > When creating cgroup hierarchy we need to enable controllers in the > > parent cgroup in order to be usable. That means writing "+{controller}" > > into cgroup.subtree_control file. We can enable only controllers that > > are enabled for parent cgroup, that means we need to do that for the > > whole cgroup tree. > > > > Cgroups for threads needs to be handled differently in cgroup v2. There > > are two types of controllers: > > > > - domain controllers: these cannot be enabled for threads > > - threaded controllers: these can be enabled for threads > > > > In addition there are multiple types of cgroups: > > > > - domain: normal cgroup > > - domain threaded: a domain cgroup that serves as root for threaded > > cgroups > > - domain invalid: invalid cgroup, can be changed into threaded, this > > is the default state if you create subgroup inside > > domain threaded group or threaded group > > - threaded: threaded cgroup which can have domain threaded or > > threaded as parent group > > > > In order to create threaded cgroup it's sufficient to write "threaded" > > into cgroup.type file, it will automatically make parent cgroup > > "domain threaded" if it was only "domain". In case the parent cgroup > > is already "domain threaded" or "threaded" it will modify only the type > > of current cgroup. After that we can enable threaded controllers. > > > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > > --- > > src/util/vircgroup.c | 2 +- > > src/util/vircgroupbackend.h | 1 + > > src/util/vircgroupv2.c | 78 +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 80 insertions(+), 1 deletion(-) > > > > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > > index 1097b1f998..dc249bfe33 100644 > > --- a/src/util/vircgroup.c > > +++ b/src/util/vircgroup.c > > @@ -938,7 +938,7 @@ virCgroupNewThread(virCgroupPtr domain, > > if (virCgroupNew(-1, name, domain, controllers, group) < 0) > > return -1; > > > > - if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_NONE) < 0) { > > + if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_THREAD) < 0) { > > So unsupported flags are ignored? Correct, each backend will create the directories differently and there are different rules applies, so the flag can be ignored if there is no need to handle it. > > virCgroupFree(group); > > return -1; > > } > > diff --git a/src/util/vircgroupbackend.h b/src/util/vircgroupbackend.h > > index b1f19233e4..86d1539e07 100644 > > --- a/src/util/vircgroupbackend.h > > +++ b/src/util/vircgroupbackend.h > > @@ -33,6 +33,7 @@ typedef enum { > > * before creating subcgroups and > > * attaching tasks > > */ > > + VIR_CGROUP_THREAD = 1 << 1, /* cgroup v2 handles threads differently */ > > } virCgroupBackendFlags; > > > > typedef enum { > > diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c > > index 3ca463e4c2..8fb9ace474 100644 > > --- a/src/util/vircgroupv2.c > > +++ b/src/util/vircgroupv2.c > > @@ -336,6 +336,83 @@ virCgroupV2PathOfController(virCgroupPtr group, > > } > > > > > > +static int > > +virCgroupV2EnableController(virCgroupPtr parent, > > + int controller) > > +{ > > + VIR_AUTOFREE(char *) val = NULL; > > + > > + if (virAsprintf(&val, "+%s", > > + virCgroupV2ControllerTypeToString(controller)) < 0) { > > + return -1; > > + } > > + > > + if (virCgroupSetValueStr(parent, controller, > > + "cgroup.subtree_control", val) < 0) { > > + return -1; > > + } > > + > > + return 0; > > +} > > + > > + > > +static int > > +virCgroupV2MakeGroup(virCgroupPtr parent ATTRIBUTE_UNUSED, > > + virCgroupPtr group, > > + bool create, > > + unsigned int flags) > > +{ > > + VIR_AUTOFREE(char *) path = NULL; > > + int controller; > > + > > + VIR_DEBUG("Make group %s", group->path); > > + > > + controller = virCgroupV2GetAnyController(group); > > + if (virCgroupV2PathOfController(group, controller, "", &path) < 0) > > + return -1; > > + > > + VIR_DEBUG("Make controller %s", path); > > + > > + if (!virFileExists(path)) { > > This should have been virFileIsDir() because EEXIST doesn't mean the > @path is a directory. It could be a regular file. Good point, I'll fix that and as a followup we should fix it for cgroup v1 backend as well. > > + if (!create || mkdir(path, 0755 || errno == EEXIST) < 0) { > > Ooops, misplaced ')'. mkdir() || errno ;-) This is why I tend to write > each condition on a separate line. But if you want to ignore EEXIST then > you need to rewrite this check as follows: > > if (!create || (mkdir() < 0 && errno != EEXIST)) > > However, I don't think you want to ignore any errno. The point of EEXIST was the there can be race condition when file creation so in case mkdir() fails with EEXIST we are ignoring that error. The race condition may happen if you start multiple guests on non-systemd OS where libvirt is actually creating the whole hierarchy and you need to create the /machines partition directory itself (my guess, I did not verify it). > Also, any reason these two if()-s should not be merged into one? No reason, they can be merged. > > > + virReportSystemError(errno, > > + _("Failed to create v2 cgroup '%s'"), > > + group->path); > > + return -1; > > + } > > + } > > + > > + if (create) { > > + if (flags & VIR_CGROUP_THREAD) { > > + if (virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_CPU, > > + "cgroup.type", "threaded") < 0) { > > + return -1; > > + } > > + > > + if (virCgroupV2EnableController(parent, > > + VIR_CGROUP_CONTROLLER_CPU) < 0) { > > + return -1; > > + } > > + } else { > > + size_t i; > > + for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { > > + if (!virCgroupV2HasController(parent, i)) > > + continue; > > + > > + /* Controllers that are implicitly enabled if available. */ > > + if (i == VIR_CGROUP_CONTROLLER_CPUACCT) > > + continue; > > + > > + if (virCgroupV2EnableController(parent, i) < 0) > > + return -1; > > + } > > + } > > + } > > + > > + return 0; > > +} > > + > > + > > virCgroupBackend virCgroupV2Backend = { > > .type = VIR_CGROUP_BACKEND_TYPE_V2, > > > > @@ -351,6 +428,7 @@ virCgroupBackend virCgroupV2Backend = { > > .hasController = virCgroupV2HasController, > > .getAnyController = virCgroupV2GetAnyController, > > .pathOfController = virCgroupV2PathOfController, > > + .makeGroup = virCgroupV2MakeGroup, > > }; > > > > > > > > Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Oct 04, 2018 at 03:20:18PM +0200, Pavel Hrdina wrote: > On Thu, Oct 04, 2018 at 02:55:25PM +0200, Michal Privoznik wrote: > > On 10/02/2018 10:43 AM, Pavel Hrdina wrote: > > > When creating cgroup hierarchy we need to enable controllers in the > > > parent cgroup in order to be usable. That means writing "+{controller}" > > > into cgroup.subtree_control file. We can enable only controllers that > > > are enabled for parent cgroup, that means we need to do that for the > > > whole cgroup tree. > > > > > > Cgroups for threads needs to be handled differently in cgroup v2. There > > > are two types of controllers: > > > > > > - domain controllers: these cannot be enabled for threads > > > - threaded controllers: these can be enabled for threads > > > > > > In addition there are multiple types of cgroups: > > > > > > - domain: normal cgroup > > > - domain threaded: a domain cgroup that serves as root for threaded > > > cgroups > > > - domain invalid: invalid cgroup, can be changed into threaded, this > > > is the default state if you create subgroup inside > > > domain threaded group or threaded group > > > - threaded: threaded cgroup which can have domain threaded or > > > threaded as parent group > > > > > > In order to create threaded cgroup it's sufficient to write "threaded" > > > into cgroup.type file, it will automatically make parent cgroup > > > "domain threaded" if it was only "domain". In case the parent cgroup > > > is already "domain threaded" or "threaded" it will modify only the type > > > of current cgroup. After that we can enable threaded controllers. > > > > > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > > > --- > > > src/util/vircgroup.c | 2 +- > > > src/util/vircgroupbackend.h | 1 + > > > src/util/vircgroupv2.c | 78 +++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 80 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > > > index 1097b1f998..dc249bfe33 100644 > > > --- a/src/util/vircgroup.c > > > +++ b/src/util/vircgroup.c > > > @@ -938,7 +938,7 @@ virCgroupNewThread(virCgroupPtr domain, > > > if (virCgroupNew(-1, name, domain, controllers, group) < 0) > > > return -1; > > > > > > - if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_NONE) < 0) { > > > + if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_THREAD) < 0) { > > > > So unsupported flags are ignored? > > Correct, each backend will create the directories differently and there > are different rules applies, so the flag can be ignored if there is no > need to handle it. > > > > virCgroupFree(group); > > > return -1; > > > } > > > diff --git a/src/util/vircgroupbackend.h b/src/util/vircgroupbackend.h > > > index b1f19233e4..86d1539e07 100644 > > > --- a/src/util/vircgroupbackend.h > > > +++ b/src/util/vircgroupbackend.h > > > @@ -33,6 +33,7 @@ typedef enum { > > > * before creating subcgroups and > > > * attaching tasks > > > */ > > > + VIR_CGROUP_THREAD = 1 << 1, /* cgroup v2 handles threads differently */ > > > } virCgroupBackendFlags; > > > > > > typedef enum { > > > diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c > > > index 3ca463e4c2..8fb9ace474 100644 > > > --- a/src/util/vircgroupv2.c > > > +++ b/src/util/vircgroupv2.c > > > @@ -336,6 +336,83 @@ virCgroupV2PathOfController(virCgroupPtr group, > > > } > > > > > > > > > +static int > > > +virCgroupV2EnableController(virCgroupPtr parent, > > > + int controller) > > > +{ > > > + VIR_AUTOFREE(char *) val = NULL; > > > + > > > + if (virAsprintf(&val, "+%s", > > > + virCgroupV2ControllerTypeToString(controller)) < 0) { > > > + return -1; > > > + } > > > + > > > + if (virCgroupSetValueStr(parent, controller, > > > + "cgroup.subtree_control", val) < 0) { > > > + return -1; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > + > > > +static int > > > +virCgroupV2MakeGroup(virCgroupPtr parent ATTRIBUTE_UNUSED, > > > + virCgroupPtr group, > > > + bool create, > > > + unsigned int flags) > > > +{ > > > + VIR_AUTOFREE(char *) path = NULL; > > > + int controller; > > > + > > > + VIR_DEBUG("Make group %s", group->path); > > > + > > > + controller = virCgroupV2GetAnyController(group); > > > + if (virCgroupV2PathOfController(group, controller, "", &path) < 0) > > > + return -1; > > > + > > > + VIR_DEBUG("Make controller %s", path); > > > + > > > + if (!virFileExists(path)) { > > > > This should have been virFileIsDir() because EEXIST doesn't mean the > > @path is a directory. It could be a regular file. > > Good point, I'll fix that and as a followup we should fix it for cgroup > v1 backend as well. Actually it is guaranteed that the path will be only DIR, cgroup fs doesn't allow to create regular files and when creating partitions we check for collisions with cgroup interface files so virFileExists is good enough. Pavel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.