[libvirt] [PATCH] Fix nodeinfo output on PPC64 KVM hosts
The nodeinfo is reporting incorrect number of cpus and incorrect host topology on PPC64 KVM hosts. The KVM hypervisor on PPC64 needs only the primary thread in a core to be online, and the secondaries offlined. While scheduling a guest in, the kvm scheduler wakes up the secondaries to run in guest context. The host scheduling of the guests happen at the core level(as only primary thread is online). The kvm scheduler exploits as many threads of the core as needed by guest. Further, starting POWER8, the processor allows splitting a physical core into multiple subcores with 2 or 4 threads each. Again, only the primary thread in a subcore is online in the host. The KVM-PPC scheduler allows guests to exploit all the offline threads in the subcore, by bringing them online when needed. (Kernel patches on split-core http://www.spinics.net/lists/kvm-ppc/msg09121.html) Recently with dynamic micro-threading changes in ppc-kvm, makes sure to utilize all the offline cpus across guests, and across guests with different cpu topologies. (https://www.mail-archive.com/kvm@vger.kernel.org/msg115978.html) Since the offline cpus are brought online in the guest context, it is safe to count them as online. Nodeinfo today discounts these offline cpus from cpu count/topology calclulation, and the nodeinfo output is not of any help and the host appears overcommited when it is actually not. The patch carefully counts those offline threads whose primary threads are online. The host topology displayed by the nodeinfo is also fixed when the host is in valid kvm state. Test results with and without fix: http://ur1.ca/mx25y Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/nodeinfo.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 2fafe2d..1a27080 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -32,6 +32,8 @@ #include <sys/utsname.h> #include <sched.h> #include "conf/domain_conf.h" +#include <fcntl.h> +#include <sys/ioctl.h> #if defined(__FreeBSD__) || defined(__APPLE__) # include <sys/time.h> @@ -428,6 +430,10 @@ virNodeParseNode(const char *node, unsigned int cpu; int online; int direrr; + int kvmfd = -1; + int lastonline; + int threads_per_subcore = 1; + bool valid_ppc64_kvmhost_mode = false; *threads = 0; *cores = 0; @@ -464,6 +470,66 @@ virNodeParseNode(const char *node, sock_max++; + if (ARCH_IS_PPC64(arch)) { + /* PPC-KVM needs the secondary threads of a core to be offline on the + * host. The kvm schedular brings the secondary threads online in the + * guest context. Moreover, P8 processor has split-core capability + * where, there can be 1,2 or 4 subcores per core. The primaries of the + * subcores alone will be online on the host for a subcore in the + * host. Even though the actual threads per core for P8 processor is 8, + * depending on the subcores_per_core = 1, 2 or 4, the threads per + * subcore will vary accordingly to 8, 4 and 2 repectively. + * So, On host threads_per_core what is arrived at from sysfs in the + * current logic is actually the subcores_per_core. Threads per subcore + * can only be obtained from the kvm device. For example, on P8 wih 1 + * core having 8 threads, sub_cores_percore=4, the threads 0,2,4 & 6 + * will be online. The sysfs reflects this and in the current logic + * variable 'threads' will be 4 which is nothing but subcores_per_core. + * If the user tampers the cpu online/offline states using chcpu or other + * means, then it is an unsupported configuration for kvm. + * The code below tries to keep in mind + * - when the libvirtd is run inside a KVM guest or Phyp based guest. + * - Or on the kvm host where user manually tampers the cpu states to + * offline/online randomly. + */ +# if HAVE_LINUX_KVM_H +# include <linux/kvm.h> + 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 */ + break; + } + lastonline = cpu; + } + } + } + /* 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) { + if (online) { + lastonline = cpu; + } else if (lastonline != -1 && + (cpu-lastonline) < threads_per_subcore) { + processors++; /* Count only those secondaries whose primary + subcore is online */ + continue; + } + } + if (!online) { (*offline)++; continue; @@ -528,6 +606,13 @@ virNodeParseNode(const char *node, *cores = core; } + if (ARCH_IS_PPC64(arch) && valid_ppc64_kvmhost_mode) { + /* 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:
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
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@redhat.com> 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
On Wed, 2015-07-01 at 19:26 +0530, Shivaprasad bhat wrote:
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.
Only commit 1/2 seems to have made it to the list... Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Wed, 2015-07-01 at 19:26 +0530, Shivaprasad bhat wrote:
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.
Hi Shivaprasad, I've just posted a reworked patch set; the changes are described in the cover letter. Can you please take a look at it and let me know if it looks okay to you? Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
participants (3)
-
Andrea Bolognani -
Shivaprasad bhat -
Shivaprasad G Bhat