[libvirt] [PATCHv2 0/3] reorder qemu cgroups operations
This is a much shorter series focusing on the key point, the second patch. The first patch is somehing that was found when looking at the code and is just a cosmetic change. The third patch just cleans up. They where both already ACKed. Patch 2 was also already ACKed but conflicted with another pending change. It should be reviewed in its new context. Note the new order with the "manual" affinity setting code. @Peter: qemuProcessInitCpuAffinity and qemuProcessSetupEmulator have a lot in in common. I guess there is potential for further simplification. The series is based on 92ec2e5e9b79b7df4d575040224bd606ab0b6dd8 with these two patches on top: http://www.redhat.com/archives/libvir-list/2016-February/msg01211.html Henning Schild (3): vircgroup: one central point for adding tasks to cgroups qemu_cgroup: put qemu right into emulator sub-cgroup qemu_cgroup: use virCgroupAddTask instead of virCgroupMoveTask src/libvirt_private.syms | 1 - src/qemu/qemu_process.c | 10 ++--- src/util/vircgroup.c | 105 +---------------------------------------------- src/util/vircgroup.h | 3 -- 4 files changed, 6 insertions(+), 113 deletions(-) -- 2.4.10
Use virCgroupAddTaskController in virCgroupAddTask so we have one single point where we add tasks to cgroups. Signed-off-by: Henning Schild <henning.schild@siemens.com> --- src/util/vircgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 6ce208e..ec59150 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1183,7 +1183,7 @@ virCgroupAddTask(virCgroupPtr group, pid_t pid) if (i == VIR_CGROUP_CONTROLLER_SYSTEMD) continue; - if (virCgroupSetValueU64(group, i, "tasks", pid) < 0) + if (virCgroupAddTaskController(group, pid, i) < 0) goto cleanup; } -- 2.4.10
Move qemuProcessSetupEmulator up under qemuSetupCgroup. That way we move the one main thread right into the emulator cgroup, instead of moving multiple threads later on. And we do not actually want any threads running in the parent cgroups (cpu cpuacct cpuset). Signed-off-by: Henning Schild <henning.schild@siemens.com> --- src/qemu/qemu_process.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0c43183..7725a5f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5087,6 +5087,10 @@ qemuProcessLaunch(virConnectPtr conn, qemuProcessInitCpuAffinity(vm) < 0) goto cleanup; + VIR_DEBUG("Setting emulator tuning/settings"); + if (qemuProcessSetupEmulator(vm) < 0) + goto cleanup; + VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, vm->def, @@ -5129,10 +5133,6 @@ qemuProcessLaunch(virConnectPtr conn, if (rv == -1) /* The VM failed to start */ goto cleanup; - VIR_DEBUG("Setting emulator tuning/settings"); - if (qemuProcessSetupEmulator(vm) < 0) - goto cleanup; - VIR_DEBUG("Waiting for monitor to show up"); if (qemuProcessWaitForMonitor(driver, vm, asyncJob, priv->qemuCaps, logCtxt) < 0) goto cleanup; -- 2.4.10
On Fri, Feb 26, 2016 at 04:34:23PM +0100, Henning Schild wrote:
Move qemuProcessSetupEmulator up under qemuSetupCgroup. That way we move the one main thread right into the emulator cgroup, instead of moving multiple threads later on. And we do not actually want any threads running in the parent cgroups (cpu cpuacct cpuset).
Signed-off-by: Henning Schild <henning.schild@siemens.com> --- src/qemu/qemu_process.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0c43183..7725a5f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5087,6 +5087,10 @@ qemuProcessLaunch(virConnectPtr conn, qemuProcessInitCpuAffinity(vm) < 0) goto cleanup;
+ VIR_DEBUG("Setting emulator tuning/settings"); + if (qemuProcessSetupEmulator(vm) < 0) + goto cleanup; + VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, vm->def, @@ -5129,10 +5133,6 @@ qemuProcessLaunch(virConnectPtr conn, if (rv == -1) /* The VM failed to start */ goto cleanup;
- VIR_DEBUG("Setting emulator tuning/settings"); - if (qemuProcessSetupEmulator(vm) < 0) - goto cleanup; - VIR_DEBUG("Waiting for monitor to show up"); if (qemuProcessWaitForMonitor(driver, vm, asyncJob, priv->qemuCaps, logCtxt) < 0) goto cleanup;
Do you have some other local patches applied to your git ? I just went to apply this and realized that qemuProcessSetupEmulator() does not actually exist. It git master the function is qemuSetupCgroupForEmulator and there is another function call qemuProcessSetEmulatorAffinity just after it too. So I can't apply this patch Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Mar 01, 2016 at 11:20:18 +0000, Daniel Berrange wrote:
On Fri, Feb 26, 2016 at 04:34:23PM +0100, Henning Schild wrote:
Move qemuProcessSetupEmulator up under qemuSetupCgroup. That way we move the one main thread right into the emulator cgroup, instead of moving multiple threads later on. And we do not actually want any threads running in the parent cgroups (cpu cpuacct cpuset).
Signed-off-by: Henning Schild <henning.schild@siemens.com> --- src/qemu/qemu_process.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0c43183..7725a5f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5087,6 +5087,10 @@ qemuProcessLaunch(virConnectPtr conn, qemuProcessInitCpuAffinity(vm) < 0) goto cleanup;
+ VIR_DEBUG("Setting emulator tuning/settings"); + if (qemuProcessSetupEmulator(vm) < 0) + goto cleanup; + VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, vm->def, @@ -5129,10 +5133,6 @@ qemuProcessLaunch(virConnectPtr conn, if (rv == -1) /* The VM failed to start */ goto cleanup;
- VIR_DEBUG("Setting emulator tuning/settings"); - if (qemuProcessSetupEmulator(vm) < 0) - goto cleanup; - VIR_DEBUG("Waiting for monitor to show up"); if (qemuProcessWaitForMonitor(driver, vm, asyncJob, priv->qemuCaps, logCtxt) < 0) goto cleanup;
Do you have some other local patches applied to your git ? I just went to apply this and realized that qemuProcessSetupEmulator() does not actually exist. It git master the function is qemuSetupCgroupForEmulator and there is another function call qemuProcessSetEmulatorAffinity just after it too. So I can't apply this patch
This was based on top of my refactor that creates qemuProcessSetupEmulator. I didn't realize Henning based that on top of my patch. I wanted to wait until this gets sorted and then re-do my patch, but I can push it so that you can apply that. Peter
On Tue, 1 Mar 2016 12:34:44 +0100 Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Mar 01, 2016 at 11:20:18 +0000, Daniel Berrange wrote:
On Fri, Feb 26, 2016 at 04:34:23PM +0100, Henning Schild wrote:
Move qemuProcessSetupEmulator up under qemuSetupCgroup. That way we move the one main thread right into the emulator cgroup, instead of moving multiple threads later on. And we do not actually want any threads running in the parent cgroups (cpu cpuacct cpuset).
Signed-off-by: Henning Schild <henning.schild@siemens.com> --- src/qemu/qemu_process.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0c43183..7725a5f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5087,6 +5087,10 @@ qemuProcessLaunch(virConnectPtr conn, qemuProcessInitCpuAffinity(vm) < 0) goto cleanup;
+ VIR_DEBUG("Setting emulator tuning/settings"); + if (qemuProcessSetupEmulator(vm) < 0) + goto cleanup; + VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, vm->def, @@ -5129,10 +5133,6 @@ qemuProcessLaunch(virConnectPtr conn, if (rv == -1) /* The VM failed to start */ goto cleanup;
- VIR_DEBUG("Setting emulator tuning/settings"); - if (qemuProcessSetupEmulator(vm) < 0) - goto cleanup; - VIR_DEBUG("Waiting for monitor to show up"); if (qemuProcessWaitForMonitor(driver, vm, asyncJob, priv->qemuCaps, logCtxt) < 0) goto cleanup;
Do you have some other local patches applied to your git ? I just went to apply this and realized that qemuProcessSetupEmulator() does not actually exist. It git master the function is qemuSetupCgroupForEmulator and there is another function call qemuProcessSetEmulatorAffinity just after it too. So I can't apply this patch
This was based on top of my refactor that creates qemuProcessSetupEmulator. I didn't realize Henning based that on top of my patch. I wanted to wait until this gets sorted and then re-do my patch, but I can push it so that you can apply that.
Peter could you please look into the affinity setting code of qemu after my patch 2? In the cgroups affinity setting code the main threads (qemuProcess) is considered an emulator-thread right away. The manual affinty setting code should apply the same scheme. I think qemuProcessInitCpuAffinity is obsolte. If cornercases remain they should become part of qemuProcessSetupEmulator. Depending on how qemuProcessSetupEmulator has to change we will see about the patch ordering later.
Peter
On Tue, Mar 01, 2016 at 12:34:44PM +0100, Peter Krempa wrote:
On Tue, Mar 01, 2016 at 11:20:18 +0000, Daniel Berrange wrote:
On Fri, Feb 26, 2016 at 04:34:23PM +0100, Henning Schild wrote:
Move qemuProcessSetupEmulator up under qemuSetupCgroup. That way we move the one main thread right into the emulator cgroup, instead of moving multiple threads later on. And we do not actually want any threads running in the parent cgroups (cpu cpuacct cpuset).
Signed-off-by: Henning Schild <henning.schild@siemens.com> --- src/qemu/qemu_process.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0c43183..7725a5f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5087,6 +5087,10 @@ qemuProcessLaunch(virConnectPtr conn, qemuProcessInitCpuAffinity(vm) < 0) goto cleanup;
+ VIR_DEBUG("Setting emulator tuning/settings"); + if (qemuProcessSetupEmulator(vm) < 0) + goto cleanup; + VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, vm->def, @@ -5129,10 +5133,6 @@ qemuProcessLaunch(virConnectPtr conn, if (rv == -1) /* The VM failed to start */ goto cleanup;
- VIR_DEBUG("Setting emulator tuning/settings"); - if (qemuProcessSetupEmulator(vm) < 0) - goto cleanup; - VIR_DEBUG("Waiting for monitor to show up"); if (qemuProcessWaitForMonitor(driver, vm, asyncJob, priv->qemuCaps, logCtxt) < 0) goto cleanup;
Do you have some other local patches applied to your git ? I just went to apply this and realized that qemuProcessSetupEmulator() does not actually exist. It git master the function is qemuSetupCgroupForEmulator and there is another function call qemuProcessSetEmulatorAffinity just after it too. So I can't apply this patch
This was based on top of my refactor that creates qemuProcessSetupEmulator. I didn't realize Henning based that on top of my patch. I wanted to wait until this gets sorted and then re-do my patch, but I can push it so that you can apply that.
Rather than go through another rebase, I've just pushed your patch and Henning's on top of it Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, 1 Mar 2016 11:20:18 +0000 "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Fri, Feb 26, 2016 at 04:34:23PM +0100, Henning Schild wrote:
Move qemuProcessSetupEmulator up under qemuSetupCgroup. That way we move the one main thread right into the emulator cgroup, instead of moving multiple threads later on. And we do not actually want any threads running in the parent cgroups (cpu cpuacct cpuset).
Signed-off-by: Henning Schild <henning.schild@siemens.com> --- src/qemu/qemu_process.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0c43183..7725a5f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5087,6 +5087,10 @@ qemuProcessLaunch(virConnectPtr conn, qemuProcessInitCpuAffinity(vm) < 0) goto cleanup;
+ VIR_DEBUG("Setting emulator tuning/settings"); + if (qemuProcessSetupEmulator(vm) < 0) + goto cleanup; + VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, vm->def, @@ -5129,10 +5133,6 @@ qemuProcessLaunch(virConnectPtr conn, if (rv == -1) /* The VM failed to start */ goto cleanup;
- VIR_DEBUG("Setting emulator tuning/settings"); - if (qemuProcessSetupEmulator(vm) < 0) - goto cleanup; - VIR_DEBUG("Waiting for monitor to show up"); if (qemuProcessWaitForMonitor(driver, vm, asyncJob, priv->qemuCaps, logCtxt) < 0) goto cleanup;
Do you have some other local patches applied to your git ? I just went to apply this and realized that qemuProcessSetupEmulator() does not actually exist. It git master the function is qemuSetupCgroupForEmulator and there is another function call qemuProcessSetEmulatorAffinity just after it too. So I can't apply this patch
Yes i have two other patches that John also wanted to merge. They would have conflicted with mine. See cover letter of this series.
Regards, Daniel
qemuProcessSetupEmulator runs at a point in time where there is only the qemu main thread. Use virCgroupAddTask to put just that one task into the emulator cgroup. That patch makes virCgroupMoveTask and virCgroupAddTaskStrController obsolete. Signed-off-by: Henning Schild <henning.schild@siemens.com> --- src/libvirt_private.syms | 1 - src/qemu/qemu_process.c | 2 +- src/util/vircgroup.c | 103 ----------------------------------------------- src/util/vircgroup.h | 3 -- 4 files changed, 1 insertion(+), 108 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4cfaed5..a318cb2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1233,7 +1233,6 @@ virCgroupHasEmptyTasks; virCgroupKill; virCgroupKillPainfully; virCgroupKillRecursive; -virCgroupMoveTask; virCgroupNewDetect; virCgroupNewDetectMachine; virCgroupNewDomainPartition; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7725a5f..08a9eb6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2233,7 +2233,7 @@ qemuProcessSetupEmulator(virDomainObjPtr vm) true, &cgroup_emulator) < 0) goto cleanup; - if (virCgroupMoveTask(priv->cgroup, cgroup_emulator) < 0) + if (virCgroupAddTask(cgroup_emulator, vm->pid) < 0) goto cleanup; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index ec59150..42276ca 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1224,99 +1224,6 @@ virCgroupAddTaskController(virCgroupPtr group, pid_t pid, int controller) static int -virCgroupAddTaskStrController(virCgroupPtr group, - const char *pidstr, - int controller) -{ - char *str = NULL, *cur = NULL, *next = NULL; - unsigned long long p = 0; - int rc = 0; - char *endp; - - if (VIR_STRDUP(str, pidstr) < 0) - return -1; - - cur = str; - while (*cur != '\0') { - if (virStrToLong_ull(cur, &endp, 10, &p) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse '%s' as an integer"), cur); - goto cleanup; - } - - if (virCgroupAddTaskController(group, p, controller) < 0) { - /* A thread that exits between when we first read the source - * tasks and now is not fatal. */ - if (virLastErrorIsSystemErrno(ESRCH)) - virResetLastError(); - else - goto cleanup; - } - - next = strchr(cur, '\n'); - if (next) { - cur = next + 1; - *next = '\0'; - } else { - break; - } - } - - cleanup: - VIR_FREE(str); - return rc; -} - - -/** - * virCgroupMoveTask: - * - * @src_group: The source cgroup where all tasks are removed from - * @dest_group: The destination where all tasks are added to - * - * Returns: 0 on success or -1 on failure - */ -int -virCgroupMoveTask(virCgroupPtr src_group, virCgroupPtr dest_group) -{ - int ret = -1; - char *content = NULL; - size_t i; - - for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { - if (!src_group->controllers[i].mountPoint || - !dest_group->controllers[i].mountPoint) - continue; - - /* We must never move tasks in systemd's hierarchy */ - if (i == VIR_CGROUP_CONTROLLER_SYSTEMD) - continue; - - /* New threads are created in the same group as their parent; - * but if a thread is created after we first read we aren't - * aware that it needs to move. Therefore, we must iterate - * until content is empty. */ - while (1) { - VIR_FREE(content); - if (virCgroupGetValueStr(src_group, i, "tasks", &content) < 0) - return -1; - - if (!*content) - break; - - if (virCgroupAddTaskStrController(dest_group, content, i) < 0) - goto cleanup; - } - } - - ret = 0; - cleanup: - VIR_FREE(content); - return ret; -} - - -static int virCgroupSetPartitionSuffix(const char *path, char **res) { char **tokens; @@ -4356,16 +4263,6 @@ virCgroupAddTaskController(virCgroupPtr group ATTRIBUTE_UNUSED, int -virCgroupMoveTask(virCgroupPtr src_group ATTRIBUTE_UNUSED, - virCgroupPtr dest_group ATTRIBUTE_UNUSED) -{ - virReportSystemError(ENXIO, "%s", - _("Control groups not supported on this platform")); - return -1; -} - - -int virCgroupGetBlkioIoServiced(virCgroupPtr group ATTRIBUTE_UNUSED, long long *bytes_read ATTRIBUTE_UNUSED, long long *bytes_write ATTRIBUTE_UNUSED, diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index aeb641c..76ecf06 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -132,9 +132,6 @@ int virCgroupAddTaskController(virCgroupPtr group, pid_t pid, int controller); -int virCgroupMoveTask(virCgroupPtr src_group, - virCgroupPtr dest_group); - int virCgroupSetBlkioWeight(virCgroupPtr group, unsigned int weight); int virCgroupGetBlkioWeight(virCgroupPtr group, unsigned int *weight); -- 2.4.10
On Fri, Feb 26, 2016 at 04:34:21PM +0100, Henning Schild wrote:
This is a much shorter series focusing on the key point, the second patch. The first patch is somehing that was found when looking at the code and is just a cosmetic change. The third patch just cleans up. They where both already ACKed.
Patch 2 was also already ACKed but conflicted with another pending change. It should be reviewed in its new context. Note the new order with the "manual" affinity setting code.
@Peter: qemuProcessInitCpuAffinity and qemuProcessSetupEmulator have a lot in in common. I guess there is potential for further simplification.
The series is based on 92ec2e5e9b79b7df4d575040224bd606ab0b6dd8 with these two patches on top: http://www.redhat.com/archives/libvir-list/2016-February/msg01211.html
Henning Schild (3): vircgroup: one central point for adding tasks to cgroups qemu_cgroup: put qemu right into emulator sub-cgroup qemu_cgroup: use virCgroupAddTask instead of virCgroupMoveTask
src/libvirt_private.syms | 1 - src/qemu/qemu_process.c | 10 ++--- src/util/vircgroup.c | 105 +---------------------------------------------- src/util/vircgroup.h | 3 -- 4 files changed, 6 insertions(+), 113 deletions(-)
ACK to all three, but we should probably wait until after release freeze is over to push this. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Henning Schild -
Peter Krempa