Hi Andrea,
I have posted the v2 as per your suggestions. Also added test case to
test the nodeinfo. I moved the ioctl to
a new function to help mocking it for testing.
For everyone, the capabilities is reporting correctly with
numactl-devel installed as confirmed by Andrea over IRC.
Thanks and Regards,
Shiva
On Mon, Jun 29, 2015 at 5:41 PM, Andrea Bolognani <abologna@xxxxxxxxxx> wrote:
> On Thu, 2016-06-25 at 12:03 +0530, Shivaprasad G Bhat wrote:
>
> Hi Shivaprasad,
>
> here are a few comments about your patch.
>
>>
> [...]
>> +# if HAVE_LINUX_KVM_H
>> +# include <linux/kvm.h>
>
> This line should be moved to the top of the file, with all the other
> #include directives.
>
>> + kvmfd = open("/dev/kvm", O_RDONLY);
>> + if (kvmfd >= 0) {
>> +# ifdef KVM_CAP_PPC_SMT
>> + valid_ppc64_kvmhost_mode = true;
>> + /* For Phyp and KVM based guests the ioctl for
>> KVM_CAP_PPC_SMT
>> + * returns zero and both primary and secondary threads
>> will be
>> + * online. Dont try to alter or interpret anything here.
>> + */
>> + threads_per_subcore = ioctl(kvmfd, KVM_CHECK_EXTENSION,
>> KVM_CAP_PPC_SMT);
>> + if (threads_per_subcore == 0)
>> + valid_ppc64_kvmhost_mode = false;
>> +# endif
>> + }
>> + VIR_FORCE_CLOSE(kvmfd);
>> +# endif
>> + rewinddir(cpudir);
>> + lastonline = -1;
>> + while (valid_ppc64_kvmhost_mode &&
>> + ((direrr = virDirRead(cpudir, &cpudirent, node)) >
>> 0)) {
>> + if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
>> + continue;
>> + if ((online = virNodeGetCpuValue(node, cpu, "online",
>> 1)) < 0)
>> + goto cleanup;
>> +
>> + if (online) {
>> + if (lastonline != -1 &&
>> + (cpu - lastonline) < threads_per_subcore) {
>> + valid_ppc64_kvmhost_mode = false; /* if any of
>> secondaries
>> + * online */
>
> If a comment is long enough to span two lines, it should probably not
> start at the end of a statement but rather be above it.
>
>> + break;
>> + }
>> + lastonline = cpu;
>> + }
>> + }
>> + }
>> +
>
> Enclosing this whole block within the #if and #ifdef directives would
> make
> the code cleaner; on the other hand, since the skeleton of the loop is
> identical to the one a few lines above, maybe you could move the part
> reading from /dev/kvm closer to the top and merge the two loops
> together?
>
>> /* allocate cpu maps for each socket */
>> if (VIR_ALLOC_N(core_maps, sock_max) < 0)
>> goto cleanup;
>> @@ -473,6 +539,7 @@ virNodeParseNode(const char *node,
>>
>> /* iterate over all CPU's in the node */
>> rewinddir(cpudir);
>> + lastonline = -1;
>> while ((direrr = virDirRead(cpudir, &cpudirent, node)) > 0) {
>> if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
>> continue;
>> @@ -480,6 +547,17 @@ virNodeParseNode(const char *node,
>> if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) <
>> 0)
>> goto cleanup;
>>
>> + if (ARCH_IS_PPC64(arch) && valid_ppc64_kvmhost_mode) {
>
> valid_ppc64_kvmhost_mode can be true iff ARCH_IS_PPC64(arch), so this
> condition is kinda redundant.
>
>> + if (online) {
>> + lastonline = cpu;
>> + } else if (lastonline != -1 &&
>> + (cpu-lastonline) < threads_per_subcore) {
>> + processors++; /* Count only those secondaries whose
>> primary
>> + subcore is online */
>
> Same as above.
>
>> + continue;
>> + }
>> + }
>> +
>> if (!online) {
>> (*offline)++;
>> continue;
>> @@ -528,6 +606,13 @@ virNodeParseNode(const char *node,
>> *cores = core;
>> }
>>
>> + if (ARCH_IS_PPC64(arch) && valid_ppc64_kvmhost_mode) {
>
> Same as above.
>
>> + /* The actual host threads per core is
>> + * multiple of the subcores_per_core(i.e variable *threads
>> in this case)
>> + * and threads_per_subcore.
>> + */
>> + *threads = *threads * threads_per_subcore;
>> + }
>> ret = processors;
>>
>> cleanup:
>>
>
> Overall, the code makes sense and seems to work from my limited
> testing.
>
> However, this change completely breaks the topology information
> reported
> by `virsh capabilities`. Here's the output on a host with
> subcores_per_core=1, without the patch:
>
> <topology>
> <cells num='4'>
> <cell id='0'>
> <memory unit='KiB'>67108864</memory>
> <pages unit='KiB' size='64'>1048576</pages>
> <pages unit='KiB' size='16384'>0</pages>
> <pages unit='KiB' size='16777216'>0</pages>
> <distances>
> <sibling id='0' value='10'/>
> <sibling id='1' value='20'/>
> <sibling id='16' value='40'/>
> <sibling id='17' value='40'/>
> </distances>
> <cpus num='5'>
> <cpu id='0' socket_id='0' core_id='32' siblings='0'/>
> <cpu id='8' socket_id='0' core_id='40' siblings='8'/>
> <cpu id='16' socket_id='0' core_id='48' siblings='16'/>
> <cpu id='24' socket_id='0' core_id='96' siblings='24'/>
> <cpu id='32' socket_id='0' core_id='104' siblings='32'/>
> </cpus>
> </cell>
> <cell id='1'>
> <memory unit='KiB'>67108864</memory>
> <pages unit='KiB' size='64'>1048576</pages>
> <pages unit='KiB' size='16384'>0</pages>
> <pages unit='KiB' size='16777216'>0</pages>
> <distances>
> <sibling id='0' value='20'/>
> <sibling id='1' value='10'/>
> <sibling id='16' value='40'/>
> <sibling id='17' value='40'/>
> </distances>
> <cpus num='5'>
> <cpu id='40' socket_id='1' core_id='160' siblings='40'/>
> <cpu id='48' socket_id='1' core_id='168' siblings='48'/>
> <cpu id='56' socket_id='1' core_id='176' siblings='56'/>
> <cpu id='64' socket_id='1' core_id='224' siblings='64'/>
> <cpu id='72' socket_id='1' core_id='232' siblings='72'/>
> </cpus>
> </cell>
> <cell id='16'>
> <memory unit='KiB'>67108864</memory>
> <pages unit='KiB' size='64'>1048576</pages>
> <pages unit='KiB' size='16384'>0</pages>
> <pages unit='KiB' size='16777216'>0</pages>
> <distances>
> <sibling id='0' value='40'/>
> <sibling id='1' value='40'/>
> <sibling id='16' value='10'/>
> <sibling id='17' value='20'/>
> </distances>
> <cpus num='5'>
> <cpu id='80' socket_id='16' core_id='2088' siblings='80'/>
> <cpu id='88' socket_id='16' core_id='2096' siblings='88'/>
> <cpu id='96' socket_id='16' core_id='2144' siblings='96'/>
> <cpu id='104' socket_id='16' core_id='2152'
> siblings='104'/>
> <cpu id='112' socket_id='16' core_id='2160'
> siblings='112'/>
> </cpus>
> </cell>
> <cell id='17'>
> <memory unit='KiB'>67108864</memory>
> <pages unit='KiB' size='64'>1048576</pages>
> <pages unit='KiB' size='16384'>0</pages>
> <pages unit='KiB' size='16777216'>0</pages>
> <distances>
> <sibling id='0' value='40'/>
> <sibling id='1' value='40'/>
> <sibling id='16' value='20'/>
> <sibling id='17' value='10'/>
> </distances>
> <cpus num='5'>
> <cpu id='120' socket_id='17' core_id='2208'
> siblings='120'/>
> <cpu id='128' socket_id='17' core_id='2216'
> siblings='128'/>
> <cpu id='136' socket_id='17' core_id='2272'
> siblings='136'/>
> <cpu id='144' socket_id='17' core_id='2280'
> siblings='144'/>
> <cpu id='152' socket_id='17' core_id='2288'
> siblings='152'/>
> </cpus>
> </cell>
> </cells>
> </topology>
>
> With the patch:
>
> <topology>
> <cells num='1'>
> <cell id='0'>
> <memory unit='KiB'>263635136</memory>
> <cpus num='160'>
> <cpu id='0' socket_id='0' core_id='0' siblings='0'/>
> <cpu id='8' socket_id='0' core_id='1' siblings='8'/>
> <cpu id='16' socket_id='0' core_id='2' siblings='16'/>
> <cpu id='24' socket_id='0' core_id='3' siblings='24'/>
> <cpu id='32' socket_id='0' core_id='4' siblings='32'/>
> <cpu id='0'/>
> <!-- The above line is repeated 155 times -->
> </cpus>
> </cell>
> </cells>
> </topology>
>
> This issue needs to be addressed before the changes can be considered
> for inclusion. I'd also highly recommend writing tests cases in order
> to prevent regressions in the future.
>
> Cheers.
>
> --
> Andrea Bolognani
> Software Engineer - Virtualization Team
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list