Re: [libvirt] [PATCH 1/2] qemu: Add qemuProcessSetupPid()

On Wed, Jun 22, 2016 at 06:37:21PM +0200, Martin Kletzander wrote: >Setting up cgroups and other things for all kinds of threads (the >emulator thread, vCPU threads, I/O threads) was copy-pasted every time >new thing was added. Over time each one of those functions changed a >bit differently. So create one function that does all that setup and >start using it. That will shave some duplicated code and maybe fix some >bugs as well. > >Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; >--- > src/qemu/qemu_process.c | 278 +++++++++++++++--------------------------------- > 1 file changed, 87 insertions(+), 191 deletions(-) > >diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >index 215fe5f2f210..d1247d2fd0f9 100644 >--- a/src/qemu/qemu_process.c >+++ b/src/qemu/qemu_process.c >@@ -2306,70 +2306,108 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver, > } > > >+/** >+ * qemuProcessSetupPid: >+ * >+ * This function sets resource properities (affinity, cgroups, >+ * scheduler) for any PID associated with a domain. It should be used >+ * to set up emulator PIDs as well as vCPU and I/O thread pids to >+ * ensure they are all handled the same way. >+ * >+ * Returns 0 on success, -1 on error. >+ */ > static int >-qemuProcessSetupEmulator(virDomainObjPtr vm) >+qemuProcessSetupPid(virDomainObjPtr vm, >+ pid_t pid, >+ virCgroupThreadName nameval, >+ int id, >+ virBitmapPtr cpumask, >+ int sched_policy, >+ int sched_priority) You can just pass a pointer to the virDomainThreadSchedParam structure. > { >- virBitmapPtr cpumask = NULL; >- virCgroupPtr cgroup_emulator = NULL; > qemuDomainObjPrivatePtr priv = vm->privateData; >- unsigned long long period = vm->def->cputune.emulator_period; >- long long quota = vm->def->cputune.emulator_quota; >+ unsigned long long period = vm->def->cputune.period; >+ long long quota = vm->def->cputune.quota; These two are not equivalent, we even have separate XML elements for them: <cputune> <period>1000000</period> <quota>-1</quota> <emulator_period>1000000</emulator_period> <emulator_quota>-1</emulator_quota> </cputune> Also, renaming the variables and reordering the code in the respective function first would make this fragile code easier to review.

Jan