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