On Wed, 2015-07-29 at 22:30 -0400, John Ferlan wrote:
+ online_cpus = nodeGetOnlineCPUBitmap(sysfs_prefix); + if (online_cpus == NULL) + goto cleanup; + + nonline_cpus = virBitmapSize(online_cpus); + + for (cpu = 0; cpu < nonline_cpus; cpu++) { + + /* Skip primary threads */ + if (cpu % threads_per_subcore == 0) + continue; + + /* A single online subthread is enough to make the + * configuration invalid */ + if (virBitmapIsBitSet(online_cpus, cpu)) + goto cleanup; + }
Could virBitmapNextSetBit be used? Where if the returned pos (cpu) % threads_per_subcore != 0, then jump to cleanup?
I think both work, I just didn't know how large nonline_cpus could be and since the bitmap code has a way to return 'next' bit set, we could use it.
It works, and it looks nicer too! Thanks for the tip :)
+int +nodeGetThreadsPerSubcore(virArch arch) +{ + const char *kvmpath = "/dev/kvm"; + int kvmfd;
These could move inside the if below.
I'm thinking of one particular architecture where we consistently get compilation errors when there's variables that aren't used (which would be the case if HAVE_LINUX_KVM_H or KVM_CAP_PPC_SMT were not true
Done.
+ if ((kvmfd = open(kvmpath, O_RDONLY)) < 0) { + /* This can happen when running as a regular user if + * permissions are tight enough, in which case erroring out + * is better than silently falling back and reporting + * different nodeinfo depending on the user */ + virReportSystemError(errno, + _("Failed to open '%s'"), + kvmpath); + threads_per_subcore = kvmfd;
I would just set to -1 here directly rather than the return failure from open
Done.
Beyond those - things look OK to me.
Just to be sure, did you check the rest of the series as well?
Let me know if you want to make changes... Probably should wait until DV generates the release before pushing though...
I've just sent out v11 which addresses all your remarks. It should definitely only be pushed once the new release is out. Thanks for the review :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team