On 06/24/2016 05:02 AM, Martin Kletzander wrote: > 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@xxxxxxxxxx> >>> --- >>> 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? > Moving the AddTask is fine by me, I don't need to see another version. I just know from the last time I waded into the Cgroup patches water that one has to be very careful to avoid being bitten by something that lurks there. I think though your explanation and adjustment makes me feel better about the placement. If no one else has the time to look, then this is better than before... John FWIW: I think for patch 2 the ATTRIBUTE_UNUSED would have only been necessary on the qemuProcessSetupPid() - every other caller would have been using driver to call qemuProcessSetupPid -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list