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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
