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

[John Ferlan]

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 at redhat.com>
>>> ---
>>>  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