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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list