[libvirt] [PATCH 0/2] random cgroup v2 fixes
While working on cgroup v2 devices and doing some extended testing I figured out that there are two issues with the current code. Pavel Hrdina (2): vircgroup: introduce virCgroupKillRecursiveCB vircgroupv2: fix virCgroupV2ValidateMachineGroup src/util/vircgroup.c | 69 ++++++++++++++++++++----------------- src/util/vircgroupbackend.h | 7 ++++ src/util/vircgrouppriv.h | 8 +++++ src/util/vircgroupv1.c | 16 +++++++++ src/util/vircgroupv2.c | 22 ++++++++++++ 5 files changed, 90 insertions(+), 32 deletions(-) -- 2.19.2
The rewrite to support cgroup v2 missed this function. In cgroup v2 we have different files to track tasks. We would fail to remove cgroup on non-systemd OSes if there is any extra process assigned to guest cgroup because we would not kill any process form the guest cgroup. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 69 ++++++++++++++++++++----------------- src/util/vircgroupbackend.h | 7 ++++ src/util/vircgrouppriv.h | 8 +++++ src/util/vircgroupv1.c | 16 +++++++++ src/util/vircgroupv2.c | 16 +++++++++ 5 files changed, 84 insertions(+), 32 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index e20df3ea05..afb12a6436 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -2427,33 +2427,15 @@ virCgroupRemove(virCgroupPtr group) } -static int -virCgroupPathOfAnyController(virCgroupPtr group, - const char *name, - char **keypath) -{ - size_t i; - int controller; - - for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { - if (group->backends[i]) { - controller = group->backends[i]->getAnyController(group); - if (controller >= 0) - return virCgroupPathOfController(group, controller, name, keypath); - } - } - - virReportSystemError(ENOSYS, "%s", - _("No controllers are mounted")); - return -1; -} - - /* * Returns 1 if some PIDs are killed, 0 if none are killed, or -1 on error */ static int -virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr pids) +virCgroupKillInternal(virCgroupPtr group, + int signum, + virHashTablePtr pids, + int controller, + const char *taskFile) { int ret = -1; bool killedAny = false; @@ -2463,7 +2445,7 @@ virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr pids) VIR_DEBUG("group=%p path=%s signum=%d pids=%p", group, group->path, signum, pids); - if (virCgroupPathOfAnyController(group, "tasks", &keypath) < 0) + if (virCgroupPathOfController(group, controller, taskFile, &keypath) < 0) return -1; /* PIDs may be forking as we kill them, so loop @@ -2549,10 +2531,12 @@ virCgroupPidCopy(const void *name) } -static int +int virCgroupKillRecursiveInternal(virCgroupPtr group, int signum, virHashTablePtr pids, + int controller, + const char *taskFile, bool dormdir) { int ret = -1; @@ -2566,11 +2550,13 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, VIR_DEBUG("group=%p path=%s signum=%d pids=%p", group, group->path, signum, pids); - if (virCgroupPathOfAnyController(group, "", &keypath) < 0) + if (virCgroupPathOfController(group, controller, "", &keypath) < 0) return -1; - if ((rc = virCgroupKillInternal(group, signum, pids)) < 0) + if ((rc = virCgroupKillInternal(group, signum, pids, + controller, taskFile)) < 0) { goto cleanup; + } if (rc == 1) killedAny = true; @@ -2594,7 +2580,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, goto cleanup; if ((rc = virCgroupKillRecursiveInternal(subgroup, signum, pids, - true)) < 0) + controller, taskFile, true)) < 0) goto cleanup; if (rc == 1) killedAny = true; @@ -2620,8 +2606,10 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, int virCgroupKillRecursive(virCgroupPtr group, int signum) { - int ret; - VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum); + int ret = 0; + int rc; + size_t i; + virCgroupBackendPtr *backends = virCgroupBackendGetAll(); virHashTablePtr pids = virHashCreateFull(100, NULL, virCgroupPidCode, @@ -2629,10 +2617,27 @@ virCgroupKillRecursive(virCgroupPtr group, int signum) virCgroupPidCopy, NULL); - ret = virCgroupKillRecursiveInternal(group, signum, pids, false); + VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum); + if (!backends) { + ret = -1; + goto cleanup; + } + + for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { + if (backends[i]) { + rc = backends[i]->killRecursive(group, signum, pids); + if (rc < 0) { + ret = -1; + goto cleanup; + } + if (rc > 0) + ret = rc; + } + } + + cleanup: virHashFree(pids); - return ret; } diff --git a/src/util/vircgroupbackend.h b/src/util/vircgroupbackend.h index bc60b44643..a825dc4be7 100644 --- a/src/util/vircgroupbackend.h +++ b/src/util/vircgroupbackend.h @@ -24,6 +24,7 @@ # include "internal.h" # include "vircgroup.h" +# include "virhash.h" # define CGROUP_MAX_VAL 512 @@ -128,6 +129,11 @@ typedef int (*virCgroupHasEmptyTasksCB)(virCgroupPtr cgroup, int controller); +typedef int +(*virCgroupKillRecursiveCB)(virCgroupPtr group, + int signum, + virHashTablePtr pids); + typedef int (*virCgroupBindMountCB)(virCgroupPtr group, const char *oldroot, @@ -370,6 +376,7 @@ struct _virCgroupBackend { virCgroupRemoveCB remove; virCgroupAddTaskCB addTask; virCgroupHasEmptyTasksCB hasEmptyTasks; + virCgroupKillRecursiveCB killRecursive; virCgroupBindMountCB bindMount; virCgroupSetOwnerCB setOwner; diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h index 8f24b0891e..6067f5cdc8 100644 --- a/src/util/vircgrouppriv.h +++ b/src/util/vircgrouppriv.h @@ -123,4 +123,12 @@ int virCgroupNewDomainPartition(virCgroupPtr partition, int virCgroupRemoveRecursively(char *grppath); + +int virCgroupKillRecursiveInternal(virCgroupPtr group, + int signum, + virHashTablePtr pids, + int controller, + const char *taskFile, + bool dormdir); + #endif /* __VIR_CGROUP_PRIV_H__ */ diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index ab1a2870a3..45378be34b 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -757,6 +757,21 @@ virCgroupV1HasEmptyTasks(virCgroupPtr cgroup, } +static int +virCgroupV1KillRecursive(virCgroupPtr group, + int signum, + virHashTablePtr pids) +{ + int controller = virCgroupV1GetAnyController(group); + + if (controller < 0) + return -1; + + return virCgroupKillRecursiveInternal(group, signum, pids, controller, + "tasks", false); +} + + static char * virCgroupV1IdentifyRoot(virCgroupPtr group) { @@ -2041,6 +2056,7 @@ virCgroupBackend virCgroupV1Backend = { .remove = virCgroupV1Remove, .addTask = virCgroupV1AddTask, .hasEmptyTasks = virCgroupV1HasEmptyTasks, + .killRecursive = virCgroupV1KillRecursive, .bindMount = virCgroupV1BindMount, .setOwner = virCgroupV1SetOwner, diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 85aa62bd4c..dc2b2a65bc 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -462,6 +462,21 @@ virCgroupV2HasEmptyTasks(virCgroupPtr cgroup, } +static int +virCgroupV2KillRecursive(virCgroupPtr group, + int signum, + virHashTablePtr pids) +{ + int controller = virCgroupV2GetAnyController(group); + + if (controller < 0) + return -1; + + return virCgroupKillRecursiveInternal(group, signum, pids, controller, + "cgroup.threads", false); +} + + static int virCgroupV2BindMount(virCgroupPtr group, const char *oldroot, @@ -1558,6 +1573,7 @@ virCgroupBackend virCgroupV2Backend = { .remove = virCgroupV2Remove, .addTask = virCgroupV2AddTask, .hasEmptyTasks = virCgroupV2HasEmptyTasks, + .killRecursive = virCgroupV2KillRecursive, .bindMount = virCgroupV2BindMount, .setOwner = virCgroupV2SetOwner, -- 2.19.2
On 12/12/18 11:17 AM, Pavel Hrdina wrote:
The rewrite to support cgroup v2 missed this function. In cgroup v2 we have different files to track tasks.
We would fail to remove cgroup on non-systemd OSes if there is any extra process assigned to guest cgroup because we would not kill any process form the guest cgroup.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 69 ++++++++++++++++++++----------------- src/util/vircgroupbackend.h | 7 ++++ src/util/vircgrouppriv.h | 8 +++++ src/util/vircgroupv1.c | 16 +++++++++ src/util/vircgroupv2.c | 16 +++++++++ 5 files changed, 84 insertions(+), 32 deletions(-)
ACK Michal
When libvirt is reconnecting to running domain that uses cgroup v2 the QEMU process reports cgroup for the emulator directory because the main thread is in that cgroup. We need to remove the "/emulator" part in order to match with the root cgroup directory name for that domain. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroupv2.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index dc2b2a65bc..b9925967d7 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -118,6 +118,12 @@ virCgroupV2ValidateMachineGroup(virCgroupPtr group, if (virCgroupPartitionEscape(&scopename) < 0) return false; + if (!(tmp = strrchr(group->unified.placement, '/'))) + return false; + + if (STREQ(tmp, "/emulator")) + *tmp = '\0'; + if (!(tmp = strrchr(group->unified.placement, '/'))) return false; tmp++; -- 2.19.2
On 12/12/18 11:17 AM, Pavel Hrdina wrote:
When libvirt is reconnecting to running domain that uses cgroup v2 the QEMU process reports cgroup for the emulator directory because the main thread is in that cgroup. We need to remove the "/emulator" part in order to match with the root cgroup directory name for that domain.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroupv2.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index dc2b2a65bc..b9925967d7 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -118,6 +118,12 @@ virCgroupV2ValidateMachineGroup(virCgroupPtr group, if (virCgroupPartitionEscape(&scopename) < 0) return false;
+ if (!(tmp = strrchr(group->unified.placement, '/'))) + return false; + + if (STREQ(tmp, "/emulator")) + *tmp = '\0'; +
How about: if (STREQ(tmp, "/emulator")) { *tmp = '\0'; if (!(tmp = strrchr(group->unified.placement, '/'))) return false; } ACK regardless. Michal
On Thu, Dec 13, 2018 at 04:28:30PM +0100, Michal Privoznik wrote:
On 12/12/18 11:17 AM, Pavel Hrdina wrote:
When libvirt is reconnecting to running domain that uses cgroup v2 the QEMU process reports cgroup for the emulator directory because the main thread is in that cgroup. We need to remove the "/emulator" part in order to match with the root cgroup directory name for that domain.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroupv2.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index dc2b2a65bc..b9925967d7 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -118,6 +118,12 @@ virCgroupV2ValidateMachineGroup(virCgroupPtr group, if (virCgroupPartitionEscape(&scopename) < 0) return false;
+ if (!(tmp = strrchr(group->unified.placement, '/'))) + return false; + + if (STREQ(tmp, "/emulator")) + *tmp = '\0'; +
How about:
if (STREQ(tmp, "/emulator")) { *tmp = '\0';
if (!(tmp = strrchr(group->unified.placement, '/'))) return false; }
Right, that is better.
ACK regardless.
Thanks, pushed now. Pavel
participants (2)
-
Michal Privoznik -
Pavel Hrdina