From: Michal Privoznik <mprivozn@redhat.com> So far, only vircgroupv1 implements freezer controller related callbacks and both work with strings ("THAWED", "FROZEN", "FREEZING"). This works well with v1 but with CGroupsV2 there are just two states and they are represented by a number. Therefore, introduce an enum and implement enum <-> string conversion for each backend separately. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_driver.c | 15 +++++++++------ src/lxc/lxc_process.c | 3 ++- src/util/vircgroup.c | 10 ++++++---- src/util/vircgroup.h | 12 ++++++++++-- src/util/vircgroupbackend.h | 4 ++-- src/util/vircgroupv1.c | 38 +++++++++++++++++++++++++++++++------ 6 files changed, 61 insertions(+), 21 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index c0a93c0444..3ec6d298bd 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2495,11 +2495,12 @@ static int lxcFreezeContainer(virDomainObj *vm) int check_interval = 1; /* In milliseconds */ int exp = 10; int waited_time = 0; - g_autofree char *state = NULL; virLXCDomainObjPrivate *priv = vm->privateData; while (waited_time < timeout) { + virCgroupFreezerState state; int r; + /* * Writing "FROZEN" to the "freezer.state" freezes the group, * i.e., the container, temporarily transiting "FREEZING" state. @@ -2507,7 +2508,8 @@ static int lxcFreezeContainer(virDomainObj *vm) * to "FROZEN". * (see linux-2.6/Documentation/cgroups/freezer-subsystem.txt) */ - r = virCgroupSetFreezerState(priv->cgroup, "FROZEN"); + r = virCgroupSetFreezerState(priv->cgroup, + VIR_CGROUP_FREEZER_STATE_FROZEN); /* * Returning EBUSY explicitly indicates that the group is @@ -2540,9 +2542,9 @@ static int lxcFreezeContainer(virDomainObj *vm) VIR_DEBUG("Reading freezer.state failed with errno: %d", r); goto error; } - VIR_DEBUG("Read freezer.state: %s", state); + VIR_DEBUG("Read freezer.state: %d", state); - if (STREQ(state, "FROZEN")) + if (state == VIR_CGROUP_FREEZER_STATE_FROZEN) return 0; waited_time += check_interval; @@ -2563,7 +2565,7 @@ static int lxcFreezeContainer(virDomainObj *vm) * activate the group again and return an error. * This is likely to fall the group back again gracefully. */ - virCgroupSetFreezerState(priv->cgroup, "THAWED"); + virCgroupSetFreezerState(priv->cgroup, VIR_CGROUP_FREEZER_STATE_THAWED); return -1; } @@ -2643,7 +2645,8 @@ static int lxcDomainResume(virDomainPtr dom) "%s", _("domain is already running")); goto endjob; } else if (state == VIR_DOMAIN_PAUSED) { - if (virCgroupSetFreezerState(priv->cgroup, "THAWED") < 0) { + if (virCgroupSetFreezerState(priv->cgroup, + VIR_CGROUP_FREEZER_STATE_THAWED) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Resume operation failed")); goto endjob; diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index c731b28871..e22f77ef83 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -860,7 +860,8 @@ int virLXCProcessStop(virLXCDriver *driver, return -1; } - if (virCgroupSetFreezerState(priv->cgroup, "THAWED") < 0) { + if (virCgroupSetFreezerState(priv->cgroup, + VIR_CGROUP_FREEZER_STATE_THAWED) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Unable to thaw all processes")); diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 3d66d3acb2..e7c93cd010 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -2902,7 +2902,8 @@ virCgroupGetCpuacctStat(virCgroup *group, unsigned long long *user, int -virCgroupSetFreezerState(virCgroup *group, const char *state) +virCgroupSetFreezerState(virCgroup *group, + virCgroupFreezerState state) { virCgroup *parent = virCgroupGetNested(group); @@ -2912,7 +2913,8 @@ virCgroupSetFreezerState(virCgroup *group, const char *state) int -virCgroupGetFreezerState(virCgroup *group, char **state) +virCgroupGetFreezerState(virCgroup *group, + virCgroupFreezerState *state) { virCgroup *parent = virCgroupGetNested(group); @@ -3725,7 +3727,7 @@ virCgroupGetDomainTotalCpuStats(virCgroup *group G_GNUC_UNUSED, int virCgroupSetFreezerState(virCgroup *group G_GNUC_UNUSED, - const char *state G_GNUC_UNUSED) + virCgroupFreezerState state G_GNUC_UNUSED) { virReportSystemError(ENOSYS, "%s", _("Control groups not supported on this platform")); @@ -3735,7 +3737,7 @@ virCgroupSetFreezerState(virCgroup *group G_GNUC_UNUSED, int virCgroupGetFreezerState(virCgroup *group G_GNUC_UNUSED, - char **state G_GNUC_UNUSED) + virCgroupFreezerState **state G_GNUC_UNUSED) { virReportSystemError(ENOSYS, "%s", _("Control groups not supported on this platform")); diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 2a7aa3306c..6d0aa11e90 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -254,8 +254,16 @@ int virCgroupGetCpuacctPercpuUsage(virCgroup *group, char **usage); int virCgroupGetCpuacctStat(virCgroup *group, unsigned long long *user, unsigned long long *sys); -int virCgroupSetFreezerState(virCgroup *group, const char *state); -int virCgroupGetFreezerState(virCgroup *group, char **state); +typedef enum { + VIR_CGROUP_FREEZER_STATE_THAWED, + VIR_CGROUP_FREEZER_STATE_FROZEN, + VIR_CGROUP_FREEZER_STATE_FREEZING, + + VIR_CGROUP_FREEZER_STATE_LAST +} virCgroupFreezerState; + +int virCgroupSetFreezerState(virCgroup *group, virCgroupFreezerState state); +int virCgroupGetFreezerState(virCgroup *group, virCgroupFreezerState *state); int virCgroupSetCpusetMems(virCgroup *group, const char *mems); int virCgroupGetCpusetMems(virCgroup *group, char **mems); diff --git a/src/util/vircgroupbackend.h b/src/util/vircgroupbackend.h index ee2f6c1ec7..6effe49df6 100644 --- a/src/util/vircgroupbackend.h +++ b/src/util/vircgroupbackend.h @@ -339,11 +339,11 @@ typedef int typedef int (*virCgroupSetFreezerStateCB)(virCgroup *group, - const char *state); + virCgroupFreezerState state); typedef int (*virCgroupGetFreezerStateCB)(virCgroup *group, - char **state); + virCgroupFreezerState *state); typedef int (*virCgroupSetCpusetMemsCB)(virCgroup *group, diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index c1f0562249..ac05531f7d 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -2049,23 +2049,49 @@ virCgroupV1GetCpuacctStat(virCgroup *group, } +VIR_ENUM_DECL(virCgroupFreezerState); +VIR_ENUM_IMPL(virCgroupFreezerState, + VIR_CGROUP_FREEZER_STATE_LAST, + "THAWED", + "FROZEN", + "FREEZING" +); + static int virCgroupV1SetFreezerState(virCgroup *group, - const char *state) + virCgroupFreezerState state) { + const char *stateStr = virCgroupFreezerStateTypeToString(state); + return virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_FREEZER, - "freezer.state", state); + "freezer.state", stateStr); } static int virCgroupV1GetFreezerState(virCgroup *group, - char **state) + virCgroupFreezerState *state) { - return virCgroupGetValueStr(group, - VIR_CGROUP_CONTROLLER_FREEZER, - "freezer.state", state); + g_autofree char *stateStr = NULL; + int s; + + if (virCgroupGetValueStr(group, + VIR_CGROUP_CONTROLLER_FREEZER, + "freezer.state", &stateStr) < 0) { + return -1; + } + + s = virCgroupFreezerStateTypeFromString(stateStr); + if (s < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown value of freezer controller: %1$s"), + stateStr); + return -1; + } + + *state = s; + return 0; } -- 2.53.0

From: Michal Privoznik <mprivozn@redhat.com> With CGroupsV2 the freezer controller is split into two files: 1) cgroup.freeze where an integer is written to thaw(0)/freeze(1) processes within the cgroup, and 2) cgroup.events where the frozen status can be read. Now, freezing/thawing a cgroup is as simple as writing corresponding integer into cgroup.freeze. But similarly to CGroupsV1, it may take some time to actually freeze all processes inside the cgroup. So read both file and map combination of their values according to this table: | frozen from cgroup.events cgroup.freeze | 0 | 1 --------------+------------+------------- 0 | THAWED | N/A --+------------+------------- 1 | FREEZING | FROZEN Resolves: https://gitlab.com/libvirt/libvirt/-/work_items/870 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircgroupv2.c | 87 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 3fbd67fbda..1f2ba72c4d 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -1907,6 +1907,90 @@ virCgroupV2DenyAllDevices(virCgroup *group) } +static int +virCgroupV2SetFreezerState(virCgroup *group, + virCgroupFreezerState state) +{ + unsigned long long val; + + switch (state) { + case VIR_CGROUP_FREEZER_STATE_THAWED: + val = 0; break; + case VIR_CGROUP_FREEZER_STATE_FROZEN: + val = 1; break; + case VIR_CGROUP_FREEZER_STATE_FREEZING: + case VIR_CGROUP_FREEZER_STATE_LAST: + default: + virReportEnumRangeError(virCgroupFreezerState, state); + return -1; + } + + return virCgroupSetValueU64(group, + VIR_CGROUP_CONTROLLER_FREEZER, + "cgroup.freeze", + val); +} + + +static int +virCgroupV2GetFreezerState(virCgroup *group, + virCgroupFreezerState *state) +{ + unsigned long long freezeReqested; + g_autofree char *eventsStr = NULL; + const char *frozenStr = "frozen "; + const char *tmp; + + if (virCgroupGetValueU64(group, + VIR_CGROUP_CONTROLLER_FREEZER, + "cgroup.freeze", &freezeReqested) < 0) { + + return -1; + } + + if (freezeReqested == 0) { + /* No freeze is requested. */ + *state = VIR_CGROUP_FREEZER_STATE_THAWED; + return 0; + } else if (freezeReqested != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown value of freezer controller: %1$llu"), + freezeReqested); + return -1; + } + + /* Now look at cgroup.events at 'frozen' state. If it's 0, then some + * processes inside the CGroup are still being frozen. If it's 1, + * then all processes are frozen. */ + if (virCgroupGetValueStr(group, + VIR_CGROUP_CONTROLLER_FREEZER, + "cgroup.events", &eventsStr) < 0) { + return -1; + } + + if (!eventsStr || + !(tmp = strstr(eventsStr, frozenStr))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Invalid format of cgroup.events file")); + return -1; + } + + tmp += strlen(frozenStr); + if (*tmp == '0') { + *state = VIR_CGROUP_FREEZER_STATE_FREEZING; + } else if (*tmp == '1') { + *state = VIR_CGROUP_FREEZER_STATE_FROZEN; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown state of freezer controller: %1$s"), + tmp); + return -1; + } + + return 0; +} + + virCgroupBackend virCgroupV2Backend = { .type = VIR_CGROUP_BACKEND_TYPE_V2, @@ -1980,6 +2064,9 @@ virCgroupBackend virCgroupV2Backend = { .getCpusetMemoryMigrate = virCgroupV2GetCpusetMemoryMigrate, .setCpusetCpus = virCgroupV2SetCpusetCpus, .getCpusetCpus = virCgroupV2GetCpusetCpus, + + .setFreezerState = virCgroupV2SetFreezerState, + .getFreezerState = virCgroupV2GetFreezerState, }; -- 2.53.0

From: Michal Privoznik <mprivozn@redhat.com> Here's the deal: the 'devices' controller as such does not exist in CGroupsV2. The alternative is to load eBPF program that mimics the controller's behavior from CGroupsV1. But, only privileged user can load such program. This means that virt-host-validate (when ran as a regular user) claims 'devices' controller missing (rightfully so), and suggests enabling it in Kconfig. This last bit might be misleading to users [1]. Now, to fix this ideally, all three conditions should be checked (CGroupsV2, 'devices' controller and regular user), but our virCgroup module deliberately hides the version of CGroups. So check for the other two conditions. 1: https://lists.libvirt.org/archives/list/users@lists.libvirt.org/thread/USDFF... Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virt-host-validate-common.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 6bca661ffc..a0373b03ce 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -218,9 +218,17 @@ int virHostValidateCGroupControllers(const char *hvname, if (!virCgroupHasController(group, i)) { ret = VIR_VALIDATE_FAILURE(level); - virValidateFail(level, "Enable '%s' in kernel Kconfig file or " - "mount/enable cgroup controller in your system", - cg_name); + + /* Ideally we would also verify that @group is CGroupsV2, but + * our internal APIs hide that fact away, intentionally. */ + if (i == VIR_CGROUP_CONTROLLER_DEVICES && geteuid() != 0) { + virValidateFail(level, "Controller '%s' not available for unprivileged users", + cg_name); + } else { + virValidateFail(level, "Enable '%s' in kernel Kconfig file or " + "mount/enable cgroup controller in your system", + cg_name); + } } else { virValidatePass(); } -- 2.53.0