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

On Thu, Jun 23, 2016 at 03:06:50PM -0400, John Ferlan wrote: > > > On 06/22/2016 12:37 PM, 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(-) >> > > Would have been so much easier one at a time... The scroll wheel on my > mouse needs a break. > Yeah, I tried several different splits and none of them worked from my POV, maybe splitting them one by one could look better. > I think this looks so much more logical. The only other comment I have > is regarding the virCgroupAddTask call ordering... > > If I'm reading right, prior to this set of changes the emulator code > would call that right after virCgroupNewThread and then make the > qemuSetupCgroupCpusetCpus and virCgroupHasController calls while vcpus > and iothreads would call virCgroupAddTask after all cgroup related > calls. I mention this mainly because I have a feint (or faint) > recollection of there being issues when regarding ordering of calls and > what gets copied when (details I really tried to forget). > > Since emulator is the pid and vcpu/iothread are tid's I'm just being > overly cautious that there's a difference in ordering that isn't readily > apparent. > > This looks reasonable to me, but hopefully someone else will chime in > regarding the order of virCgroupNewThread, virCgroupAddTask, > qemuSetupCgroupCpusetCpus, virCgroupSetCpusetMems, and > qemuSetupCgroupVcpuBW prior to calling virProcessSetAffinity > The only difference here is whether you setup the cgroup and add the task to it after that or the other way around. And that's it, there's not more to it. There is no difference between PID/TID handling, nothing will have access to more resources because the domain is not running and adding task to a group that was not set up will be the same as keeping it where it was before the call to that function. However looking at it I see that there is a "bug" that could be made "cleaner". If we do it this way (adding task before setting the cgroup) and some of the settings fail, then the virCgroupRemove() will fail to remove that cgroup because there are tasks in it. At the end it won't boil down to anything bigger because we remove all the cgroups after qemu ends (or we kill it or whatever else). So there is error handling that could be done more nicely... So I'll change that. > Consider it a fragile ACK - > Should I wait for others, send a v2 or are you OK with me just moving the virCgroupAddTask() call after the setup?