This handles only the /proc/cpuinfo part of virHostCPUGetInfo().
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
src/util/virhostcpu.c | 60 +++++++++++++++++++++++++++++++++------------------
1 file changed, 39 insertions(+), 21 deletions(-)
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index c485a9721..4d5c56659 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -508,33 +508,17 @@ virHostCPUHasValidSubcoreConfiguration(int threads_per_subcore)
return ret;
}
-int
-virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
- virArch arch,
- unsigned int *cpus,
- unsigned int *mhz,
- unsigned int *nodes,
- unsigned int *sockets,
- unsigned int *cores,
- unsigned int *threads)
+
+static int
+virHostCPUGetInfoParseCPUInfo(FILE *cpuinfo,
+ virArch arch,
+ unsigned int *mhz)
{
- virBitmapPtr present_cpus_map = NULL;
- virBitmapPtr online_cpus_map = NULL;
char line[1024];
- DIR *nodedir = NULL;
- struct dirent *nodedirent = NULL;
- int nodecpus, nodecores, nodesockets, nodethreads, offline = 0;
- int threads_per_subcore = 0;
- unsigned int node;
int ret = -1;
- char *sysfs_nodedir = NULL;
- char *sysfs_cpudir = NULL;
- int direrr;
*mhz = 0;
- *cpus = *nodes = *sockets = *cores = *threads = 0;
- /* Start with parsing CPU clock speed from /proc/cpuinfo */
while (fgets(line, sizeof(line), cpuinfo) != NULL) {
if (ARCH_IS_X86(arch)) {
char *buf = line;
@@ -614,6 +598,40 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
}
}
+ ret = 0;
+
+ cleanup:
+ return ret;
+}
+
+int
+virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
+ virArch arch,
+ unsigned int *cpus,
+ unsigned int *mhz,
+ unsigned int *nodes,
+ unsigned int *sockets,
+ unsigned int *cores,
+ unsigned int *threads)
+{
+ virBitmapPtr present_cpus_map = NULL;
+ virBitmapPtr online_cpus_map = NULL;
+ DIR *nodedir = NULL;
+ struct dirent *nodedirent = NULL;
+ int nodecpus, nodecores, nodesockets, nodethreads, offline = 0;
+ int threads_per_subcore = 0;
+ unsigned int node;
+ int ret = -1;
+ char *sysfs_nodedir = NULL;
+ char *sysfs_cpudir = NULL;
+ int direrr;
+
+ *cpus = *nodes = *sockets = *cores = *threads = 0;
+
+ /* Start with parsing CPU clock speed from /proc/cpuinfo */
+ if (virHostCPUGetInfoParseCPUInfo(cpuinfo, arch, mhz) < 0)
+ goto cleanup;
+
/* 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();
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Andrea Bolognani <abologna@redhat.com> [2017-12-11, 05:40PM +0100]:
> This handles only the /proc/cpuinfo part of virHostCPUGetInfo().
>
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> src/util/virhostcpu.c | 60 +++++++++++++++++++++++++++++++++------------------
> 1 file changed, 39 insertions(+), 21 deletions(-)
>
> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> index c485a9721..4d5c56659 100644
> --- a/src/util/virhostcpu.c
> +++ b/src/util/virhostcpu.c
> @@ -508,33 +508,17 @@ virHostCPUHasValidSubcoreConfiguration(int threads_per_subcore)
> return ret;
> }
>
> -int
> -virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
> - virArch arch,
> - unsigned int *cpus,
> - unsigned int *mhz,
> - unsigned int *nodes,
> - unsigned int *sockets,
> - unsigned int *cores,
> - unsigned int *threads)
> +
> +static int
> +virHostCPUGetInfoParseCPUInfo(FILE *cpuinfo,
> + virArch arch,
> + unsigned int *mhz)
> {
> - virBitmapPtr present_cpus_map = NULL;
> - virBitmapPtr online_cpus_map = NULL;
> char line[1024];
> - DIR *nodedir = NULL;
> - struct dirent *nodedirent = NULL;
> - int nodecpus, nodecores, nodesockets, nodethreads, offline = 0;
> - int threads_per_subcore = 0;
> - unsigned int node;
> int ret = -1;
> - char *sysfs_nodedir = NULL;
> - char *sysfs_cpudir = NULL;
> - int direrr;
>
> *mhz = 0;
I wouldn't move this initialization.
> - *cpus = *nodes = *sockets = *cores = *threads = 0;
>
> - /* Start with parsing CPU clock speed from /proc/cpuinfo */
> while (fgets(line, sizeof(line), cpuinfo) != NULL) {
> if (ARCH_IS_X86(arch)) {
> char *buf = line;
> @@ -614,6 +598,40 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
> }
> }
>
> + ret = 0;
> +
> + cleanup:
> + return ret;
> +}
> +
> +int
> +virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
> + virArch arch,
> + unsigned int *cpus,
> + unsigned int *mhz,
> + unsigned int *nodes,
> + unsigned int *sockets,
> + unsigned int *cores,
> + unsigned int *threads)
> +{
> + virBitmapPtr present_cpus_map = NULL;
> + virBitmapPtr online_cpus_map = NULL;
> + DIR *nodedir = NULL;
> + struct dirent *nodedirent = NULL;
> + int nodecpus, nodecores, nodesockets, nodethreads, offline = 0;
> + int threads_per_subcore = 0;
> + unsigned int node;
> + int ret = -1;
> + char *sysfs_nodedir = NULL;
> + char *sysfs_cpudir = NULL;
> + int direrr;
> +
> + *cpus = *nodes = *sockets = *cores = *threads = 0;
> +
> + /* Start with parsing CPU clock speed from /proc/cpuinfo */
> + if (virHostCPUGetInfoParseCPUInfo(cpuinfo, arch, mhz) < 0)
> + goto cleanup;
Why do we cleanup here and abandon the rest of the information? Since
the information in /proc/cpuinfo is kind of volatile in its format,
shouldn't we be liberal in what we accept? If we can't parse it, we just
report mhz = 0, but gathering the rest of the information in this
function is still valuable.
> +
> /* 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();
> --
> 2.14.3
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
IBM Systems
Linux on z Systems & Virtualization Development
------------------------------------------------------------------------
IBM Deutschland
Schönaicher Str. 220
71032 Böblingen
Phone: +49 7031 16 1819
E-Mail: bwalk@de.ibm.com
------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2017-12-13 at 08:32 +0100, Bjoern Walk wrote:
> > -int
> > -virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
> > - virArch arch,
> > - unsigned int *cpus,
> > - unsigned int *mhz,
> > - unsigned int *nodes,
> > - unsigned int *sockets,
> > - unsigned int *cores,
> > - unsigned int *threads)
> > +
> > +static int
> > +virHostCPUGetInfoParseCPUInfo(FILE *cpuinfo,
> > + virArch arch,
> > + unsigned int *mhz)
> > {
> > - virBitmapPtr present_cpus_map = NULL;
> > - virBitmapPtr online_cpus_map = NULL;
> > char line[1024];
> > - DIR *nodedir = NULL;
> > - struct dirent *nodedirent = NULL;
> > - int nodecpus, nodecores, nodesockets, nodethreads, offline = 0;
> > - int threads_per_subcore = 0;
> > - unsigned int node;
> > int ret = -1;
> > - char *sysfs_nodedir = NULL;
> > - char *sysfs_cpudir = NULL;
> > - int direrr;
> >
> > *mhz = 0;
>
> I wouldn't move this initialization.
So you'd leave it in virHostCPUGetInfoPopulateLinux()? Why?
With my approach, the value is changed only in
virHostCPUGetInfoParseCPUInfo(), which makes IMHO more sense than
spreading the code that changes it across two functions.
> > + /* Start with parsing CPU clock speed from /proc/cpuinfo */
> > + if (virHostCPUGetInfoParseCPUInfo(cpuinfo, arch, mhz) < 0)
> > + goto cleanup;
>
> Why do we cleanup here and abandon the rest of the information? Since
> the information in /proc/cpuinfo is kind of volatile in its format,
> shouldn't we be liberal in what we accept? If we can't parse it, we just
> report mhz = 0, but gathering the rest of the information in this
> function is still valuable.
Most functions in libvirt either perform all tasks succesfully or
return a failure, so failing here is in line both with that and
with the existing behavior.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Andrea Bolognani <abologna@redhat.com> [2017-12-13, 12:34PM +0100]: > So you'd leave it in virHostCPUGetInfoPopulateLinux()? Why? > I found it more important that all the initialization (cpu, nodes, etc.) is in one place. But it actually doesn't matter. > With my approach, the value is changed only in > virHostCPUGetInfoParseCPUInfo(), which makes IMHO more sense than > spreading the code that changes it across two functions. > > > > + /* Start with parsing CPU clock speed from /proc/cpuinfo */ > > > + if (virHostCPUGetInfoParseCPUInfo(cpuinfo, arch, mhz) < 0) > > > + goto cleanup; > > > > Why do we cleanup here and abandon the rest of the information? Since > > the information in /proc/cpuinfo is kind of volatile in its format, > > shouldn't we be liberal in what we accept? If we can't parse it, we just > > report mhz = 0, but gathering the rest of the information in this > > function is still valuable. > > Most functions in libvirt either perform all tasks succesfully or > return a failure, so failing here is in line both with that and > with the existing behavior. > So for example for S390 we have introduced CPU frequency information in /proc/cpuinfo only recently. That means that depending on your kernel version, you either read freq. info and the rest of the stuff or you discard the whole CPU. I found this highly unintuitive. > -- > Andrea Bolognani / Red Hat / Virtualization > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > -- IBM Systems Linux on z Systems & Virtualization Development ------------------------------------------------------------------------ IBM Deutschland Schönaicher Str. 220 71032 Böblingen Phone: +49 7031 16 1819 E-Mail: bwalk@de.ibm.com ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2017-12-13 at 15:56 +0100, Bjoern Walk wrote: > > > Why do we cleanup here and abandon the rest of the information? Since > > > the information in /proc/cpuinfo is kind of volatile in its format, > > > shouldn't we be liberal in what we accept? If we can't parse it, we just > > > report mhz = 0, but gathering the rest of the information in this > > > function is still valuable. > > > > Most functions in libvirt either perform all tasks succesfully or > > return a failure, so failing here is in line both with that and > > with the existing behavior. > > So for example for S390 we have introduced CPU frequency information in > /proc/cpuinfo only recently. That means that depending on your kernel > version, you either read freq. info and the rest of the stuff or you > discard the whole CPU. I found this highly unintuitive. That's actually a very good reason! I ended up liking your approach to refactoring more than mine after all. So I made a couple of very small tweaks to it and I'm going to include it mostly as-is in my v2, coming in a minute. If you have a problem with any of my tweaks, just let me know. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.