[PATCH 0/5] virNodeGetInfo: Fix docs and propagate them to 'virsh nodeinfo'
Patches 1,2 cleanup the code the rest deals with docs. Peter Krempa (5): virHostCPUGetInfoPopulateLinux: Use automatic memory freeing virHostCPUGetInfoPopulateLinux: Remove 'cleanup' libvirt-host: Clarify/fix description of the CPU frequency field virNodeGetInfo: Improve description of the case when fake data is reported manpages: virsh: Use disclaimer from 'virNodeGetInfo()' for 'virsh nodeinfo' docs/manpages/virsh.rst | 23 ++++++++++++++++---- src/libvirt-host.c | 21 ++++++++++-------- src/util/virhostcpu.c | 47 +++++++++++++++++------------------------ 3 files changed, 50 insertions(+), 41 deletions(-) -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Use 'g_autfree' for the two temporary strings. 'sysfs_cpudir' was used in two places, one of which is in a loop. Add another helper variable for it and declare the other one in the loop. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virhostcpu.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 3b4a11effb..c4138076d9 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -644,8 +644,8 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, int threads_per_subcore = 0; unsigned int node; int ret = -1; - char *sysfs_nodedir = NULL; - char *sysfs_cpudir = NULL; + g_autofree char *sysfs_nodedir = NULL; + g_autofree char *sysfs_cpudir_fallback = NULL; int direrr; *mhz = 0; @@ -707,6 +707,8 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, threads_per_subcore = 0; while ((direrr = virDirRead(nodedir, &nodedirent, sysfs_nodedir)) > 0) { + char *sysfs_cpudir = NULL; + if (sscanf(nodedirent->d_name, "node%u", &node) != 1) continue; @@ -723,8 +725,6 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, &nodethreads, &offline)) < 0) goto cleanup; - VIR_FREE(sysfs_cpudir); - *cpus += nodecpus; if (nodesockets > *sockets) @@ -744,11 +744,9 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, goto done; fallback: - VIR_FREE(sysfs_cpudir); - - sysfs_cpudir = g_strdup_printf("%s/cpu", SYSFS_SYSTEM_PATH); + sysfs_cpudir_fallback = g_strdup_printf("%s/cpu", SYSFS_SYSTEM_PATH); - if ((nodecpus = virHostCPUParseNode(sysfs_cpudir, arch, + if ((nodecpus = virHostCPUParseNode(sysfs_cpudir_fallback, arch, present_cpus_map, online_cpus_map, threads_per_subcore, @@ -799,8 +797,6 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, ret = 0; cleanup: - VIR_FREE(sysfs_nodedir); - VIR_FREE(sysfs_cpudir); return ret; } -- 2.49.0
On 4/7/25 15:24, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Use 'g_autfree' for the two temporary strings.
'sysfs_cpudir' was used in two places, one of which is in a loop. Add another helper variable for it and declare the other one in the loop.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virhostcpu.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 3b4a11effb..c4138076d9 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -644,8 +644,8 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, int threads_per_subcore = 0; unsigned int node; int ret = -1; - char *sysfs_nodedir = NULL; - char *sysfs_cpudir = NULL; + g_autofree char *sysfs_nodedir = NULL; + g_autofree char *sysfs_cpudir_fallback = NULL; int direrr;
*mhz = 0; @@ -707,6 +707,8 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, threads_per_subcore = 0;
while ((direrr = virDirRead(nodedir, &nodedirent, sysfs_nodedir)) > 0) { + char *sysfs_cpudir = NULL;
Surely you meant g_autofree ;-)
+ if (sscanf(nodedirent->d_name, "node%u", &node) != 1) continue;
@@ -723,8 +725,6 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, &nodethreads, &offline)) < 0) goto cleanup;
- VIR_FREE(sysfs_cpudir); - *cpus += nodecpus;
if (nodesockets > *sockets) @@ -744,11 +744,9 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, goto done;
fallback: - VIR_FREE(sysfs_cpudir); - - sysfs_cpudir = g_strdup_printf("%s/cpu", SYSFS_SYSTEM_PATH); + sysfs_cpudir_fallback = g_strdup_printf("%s/cpu", SYSFS_SYSTEM_PATH);
- if ((nodecpus = virHostCPUParseNode(sysfs_cpudir, arch, + if ((nodecpus = virHostCPUParseNode(sysfs_cpudir_fallback, arch, present_cpus_map, online_cpus_map, threads_per_subcore, @@ -799,8 +797,6 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, ret = 0;
cleanup: - VIR_FREE(sysfs_nodedir); - VIR_FREE(sysfs_cpudir); return ret; }
Michal
From: Peter Krempa <pkrempa@redhat.com> As the cleanup section is empty; the code can now return directly on errors. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virhostcpu.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index c4138076d9..2c4c75731c 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -643,7 +643,6 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, int nodecpus, nodecores, nodesockets, nodethreads, offline = 0; int threads_per_subcore = 0; unsigned int node; - int ret = -1; g_autofree char *sysfs_nodedir = NULL; g_autofree char *sysfs_cpudir_fallback = NULL; int direrr; @@ -659,12 +658,11 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, /* Get information about what CPUs are present in the host and what * CPUs are online, so that we don't have to so for each node */ - present_cpus_map = virHostCPUGetPresentBitmap(); - if (!present_cpus_map) - goto cleanup; - online_cpus_map = virHostCPUGetOnlineBitmap(); - if (!online_cpus_map) - goto cleanup; + if (!(present_cpus_map = virHostCPUGetPresentBitmap())) + return -1; + + if (!(online_cpus_map = virHostCPUGetOnlineBitmap())) + return -1; /* OK, we've parsed clock speed out of /proc/cpuinfo. Get the * core, node, socket, thread and topology information from /sys @@ -699,7 +697,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, * On hosts other than POWER this will be 0, in which case a simpler * thread-counting logic will be used */ if ((threads_per_subcore = virHostCPUGetThreadsPerSubcore(arch)) < 0) - goto cleanup; + return -1; /* If the subcore configuration is not valid, just pretend subcores * are not in use and count threads one by one */ @@ -723,7 +721,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, threads_per_subcore, &nodesockets, &nodecores, &nodethreads, &offline)) < 0) - goto cleanup; + return -1; *cpus += nodecpus; @@ -738,7 +736,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, } if (direrr < 0) - goto cleanup; + return -1; if (*cpus && *nodes) goto done; @@ -752,7 +750,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, threads_per_subcore, &nodesockets, &nodecores, &nodethreads, &offline)) < 0) - goto cleanup; + return -1; *nodes = 1; *cpus = nodecpus; @@ -764,17 +762,17 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, /* There should always be at least one cpu, socket, node, and thread. */ if (*cpus == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no CPUs found")); - goto cleanup; + return -1; } if (*sockets == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no sockets found")); - goto cleanup; + return -1; } if (*threads == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no threads found")); - goto cleanup; + return -1; } /* Now check if the topology makes sense. There are machines that don't @@ -794,10 +792,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, *threads = 1; } - ret = 0; - - cleanup: - return ret; + return 0; } # define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK)) -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> The 'virNodeInfo' field for CPU frequency is named 'mhz'. The docs were mentioning 'mHZ', which is neither the field name nor proper spelling of the unit. Reword the paragraph to mention "CPU frequency" instead and explicitly name the field in virNodeInfo struct. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt-host.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libvirt-host.c b/src/libvirt-host.c index 51a1200c44..9845549e36 100644 --- a/src/libvirt-host.c +++ b/src/libvirt-host.c @@ -410,9 +410,9 @@ virConnectGetMaxVcpus(virConnectPtr conn, * Use of this API is strongly discouraged as the information provided * is not guaranteed to be accurate on all hardware platforms. * - * The mHZ value merely reflects the speed that the first CPU in the - * machine is currently running at. This speed may vary across CPUs - * and changes continually as the host OS throttles. + * The CPU frequency value (field 'mhz' in virNodeInfo) merely reflects the + * speed that the first CPU in the machine is currently running at. This speed + * may vary across CPUs and changes continually as the host OS throttles. * * The nodes/sockets/cores/threads data is potentially inaccurate as * it assumes a symmetric installation. If one NUMA node has more @@ -420,7 +420,7 @@ virConnectGetMaxVcpus(virConnectPtr conn, * wrong. It is also not able to report about CPU dies. * * Applications are recommended to use the virConnectGetCapabilities() - * call instead, which provides all the information except CPU mHZ, + * call instead, which provides all the information except CPU frequency, * in a more accurate representation. * * Returns 0 in case of success and -1 in case of failure. -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> virNodeGetInfo due to the rigid desing of the filled struct can't faithfully represent all topologies. Improve the description when that happens and outline the fallback topology. The function docs already state that users ought to use virConnectGetCapabilities() instead. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt-host.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libvirt-host.c b/src/libvirt-host.c index 9845549e36..d82ff993c2 100644 --- a/src/libvirt-host.c +++ b/src/libvirt-host.c @@ -414,10 +414,13 @@ virConnectGetMaxVcpus(virConnectPtr conn, * speed that the first CPU in the machine is currently running at. This speed * may vary across CPUs and changes continually as the host OS throttles. * - * The nodes/sockets/cores/threads data is potentially inaccurate as - * it assumes a symmetric installation. If one NUMA node has more - * sockets populated that another NUMA node this information will be - * wrong. It is also not able to report about CPU dies. + * The virNodeInfo structure is not extensible thus only supports global + * nodes/sockets/cores/threads (sockets/cores/threads is per NUMA node) + * topology information. If the host CPU has any further groupings (e.g. + * dies, clusters, etc) or the NUMA topology is non-symmetrical the structure + * can't faithfully represent the system. In such cases a fake topology + * (nodes = 1, sockets = 1, cores = number of host cpus, threads = 1) which + * only correctly represents the total host CPU count is reported. * * Applications are recommended to use the virConnectGetCapabilities() * call instead, which provides all the information except CPU frequency, -- 2.49.0
From: Peter Krempa <pkrempa@redhat.com> Adapt the disclarimer about the data not being accurate in many cases from the API docs to the virsh command using the aforementioned API. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index b609b9647f..cef9959f16 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -427,10 +427,25 @@ nodeinfo nodeinfo Returns basic information about the node, like number and type of CPU, -and size of the physical memory. The output corresponds to virNodeInfo -structure. Specifically, the "CPU socket(s)" field means number of CPU -sockets per NUMA cell. The information libvirt displays is dependent -upon what each architecture may provide. +and size of the physical memory. + +Use of this command is strongly discouraged as the information provided +is not guaranteed to be accurate on all hardware platforms. + +The *CPU frequency* value merely reflects the speed that the first CPU in the +machine is currently running at. This speed may vary across CPUs and changes +continually as the host OS throttles. + +The data structure used to fetch the data is not extensible thus only supports +global nodes/sockets/cores/threads (sockets/cores/threads is per NUMA node) +topology information. If the host CPU has any further groupings (e.g. +dies, clusters, etc) or the NUMA topology is non-symmetrical the data structure +can't faithfully represent the system. In such cases a fake topology +(nodes = 1, sockets = 1, cores = number of host cpus, threads = 1) which +only correctly represents the total host CPU count is reported. + +Recommended replacement is to use the *capabilities* command which reports +the data (except frequency) under ``/capabilities/host/topology`` XPath. nodecpumap -- 2.49.0
On 4/7/25 15:24, Peter Krempa via Devel wrote:
Patches 1,2 cleanup the code the rest deals with docs.
Peter Krempa (5): virHostCPUGetInfoPopulateLinux: Use automatic memory freeing virHostCPUGetInfoPopulateLinux: Remove 'cleanup' libvirt-host: Clarify/fix description of the CPU frequency field virNodeGetInfo: Improve description of the case when fake data is reported manpages: virsh: Use disclaimer from 'virNodeGetInfo()' for 'virsh nodeinfo'
docs/manpages/virsh.rst | 23 ++++++++++++++++---- src/libvirt-host.c | 21 ++++++++++-------- src/util/virhostcpu.c | 47 +++++++++++++++++------------------------ 3 files changed, 50 insertions(+), 41 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa