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>
> ---
> 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?
John
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)