Re: [libvirt] [PATCH 10/34] qemu: Don't use priv->ncpus to iterate cgroup setting
On 01/28/2016 08:57 AM, Peter Krempa wrote:
On Sat, Jan 16, 2016 at 10:22:13 -0500, John Ferlan wrote:
>
>
> On 01/14/2016 11:26 AM, Peter Krempa wrote:
>> Iterate over all cpus skipping inactive ones.
>> ---
>> src/qemu/qemu_driver.c | 15 ++++++++++++---
>> 1 file changed, 12 insertions(+), 3 deletions(-)
>>
>
> Patch 7 introduces virDomainDefGetVcpumap (or GetOnlineVcpuMap) - why
> not use that instead and then iterate the set bits. This works, but the
> less places that check for vcpu->online perhaps the better. Perhaps also
> a way to reduce the decision points of using ->maxvcpus or the
> virDomainDefGetVcpusMax call...
That would end up in two cases:
1) The bitmap would be recalculated before every use.
This increases complexity twofold since the function iterates the list
once to assemble the bitmap and then you iterate the bitmap to get the
objects.
2) The bitmap would need to be stored persistently
This again introduces two different places where data has to be stored.
It either then will require us to keep it in sync the two places or
remove one and the def->vcpus and def->vcpumap would need to be used in
parallel always.
I don't like either of those since the target was to remove two places
storing data about one object.
Peter
For as many places that iterate and filter based on ->online - just
figured perhaps it may work to generate and store that bitmap. When all
is said and done - will there be more places iterating defs->vcpus
without caring about online or more places that mainly care about
online? I searched on virDomainDefGetVcpu callers after the end of this
series and found 21 callers. Of those 21, I counted 17 that make the
check immediately for online. Since the index for online bitmap would be
the same as vcpus[], it just seems logical to me to utilize it. I assume
eventually vcpupids is going away and vcpus will store the pid (or -1)
for the 'online' vcpu.
As I see the code now 'virDomainDefGetVcpumap' is called from just one
place (qemuDomainGetCPUStats - patch 11) for the express purpose of
generating a bitmap of online vCPU's in order to iterate the bitmap to
determine which guestvcpus to get PercpuStats about (or more
specifically so that virCgroupGetPercpuVcpuSum only uses vcpupids array
indices that should match our online map). Conversely there is other
code that uses the virDomainDefGetVcpusMax in order to get both the
index into vcpus and vcpupids (which today are in the same order). So
the code is now getting the same data two different ways because of
course vcpupids exists.
I probably should have also noted in patch 11 review that whole fetch is
unnecessary if (start_cpu == -1), but was thinking at the time more in
terms of having the map available/stored. It's also unnecessary to
generate the bitmap if params == 0 or ncpus == 0 since it won't be used.
Again - it was a suggestion not a requirement. Just trying to provide a
perspective from someone not in the middle of making the changes...
John