[libvirt] [PATCH 14/53] vircgroup: introduce virCgroupV2MakeGroup

[Michal Privoznik]

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 at 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