1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v3 0/5] Improvements to CPU frequency reporting
  4. View patch

[libvirt] [PATCH v3 5/5] util: Don't report CPU frequency for ARM hosts

Andrea Bolognani posted 5 patches 7 years, 5 months ago
[libvirt] [PATCH v3 5/5] util: Don't report CPU frequency for ARM hosts
Posted by Andrea Bolognani 7 years, 5 months ago 
Some ARM platforms, such as the original Raspberry Pi, report the
CPU frequency in the BogoMIPS field of /proc/cpuinfo, so libvirt
parsed that field and returned it through its API.

However, not only many more boards don't report any value there,
but several - including ARMv8-based server hardware, and even the
more recent Raspberry Pi 3 - use this field as originally intended:
to report the BogoMIPS value instead of the CPU frequency.

Since we have no way of detecting how the field is being used,
it's better to report no information at all rather than something
ludicrous like "your shiny 96-core aarch64 virtualization host's
CPUs are running at a whopping 100 MHz".

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/util/virhostcpu.c                                       | 6 ++++--
 tests/virhostcpudata/linux-aarch64-rhel74-moonshot.expected | 2 +-
 tests/virhostcpudata/linux-armv6l-raspberrypi.expected      | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index 9c9f362de..1c9ee5921 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -590,12 +590,14 @@ virHostCPUParseFrequency(FILE *cpuinfo,
     const char *prefix = NULL;
     char line[1024];
 
+    /* No sensible way to retrieve CPU frequency */
+    if (ARCH_IS_ARM(arch))
+        return 0;
+
     if (ARCH_IS_X86(arch))
         prefix = "cpu MHz";
     else if (ARCH_IS_PPC(arch))
         prefix = "clock";
-    else if (ARCH_IS_ARM(arch))
-        prefix = "BogoMIPS";
 
     if (!prefix) {
         VIR_WARN("%s is not supported by the %s parser",
diff --git a/tests/virhostcpudata/linux-aarch64-rhel74-moonshot.expected b/tests/virhostcpudata/linux-aarch64-rhel74-moonshot.expected
index 24ff0ea0b..6776aa6c2 100644
--- a/tests/virhostcpudata/linux-aarch64-rhel74-moonshot.expected
+++ b/tests/virhostcpudata/linux-aarch64-rhel74-moonshot.expected
@@ -1 +1 @@
-CPUs: 8/8, MHz: 100, Nodes: 1, Sockets: 1, Cores: 8, Threads: 1
+CPUs: 8/8, MHz: 0, Nodes: 1, Sockets: 1, Cores: 8, Threads: 1
diff --git a/tests/virhostcpudata/linux-armv6l-raspberrypi.expected b/tests/virhostcpudata/linux-armv6l-raspberrypi.expected
index 146bd073e..1c4c713d5 100644
--- a/tests/virhostcpudata/linux-armv6l-raspberrypi.expected
+++ b/tests/virhostcpudata/linux-armv6l-raspberrypi.expected
@@ -1 +1 @@
-CPUs: 1/1, MHz: 697, Nodes: 1, Sockets: 1, Cores: 1, Threads: 1
+CPUs: 1/1, MHz: 0, Nodes: 1, Sockets: 1, Cores: 1, Threads: 1
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 5/5] util: Don't report CPU frequency for ARM hosts
Posted by John Ferlan 7 years, 4 months ago 

On 12/14/2017 07:34 AM, Andrea Bolognani wrote:
> Some ARM platforms, such as the original Raspberry Pi, report the
> CPU frequency in the BogoMIPS field of /proc/cpuinfo, so libvirt
> parsed that field and returned it through its API.
> 
> However, not only many more boards don't report any value there,
> but several - including ARMv8-based server hardware, and even the
> more recent Raspberry Pi 3 - use this field as originally intended:
> to report the BogoMIPS value instead of the CPU frequency.
> 
> Since we have no way of detecting how the field is being used,
> it's better to report no information at all rather than something
> ludicrous like "your shiny 96-core aarch64 virtualization host's
> CPUs are running at a whopping 100 MHz".
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/util/virhostcpu.c                                       | 6 ++++--
>  tests/virhostcpudata/linux-aarch64-rhel74-moonshot.expected | 2 +-
>  tests/virhostcpudata/linux-armv6l-raspberrypi.expected      | 2 +-
>  3 files changed, 6 insertions(+), 4 deletions(-)
> 

Suggestion - follow cmdNodeinfo for virhostcputest.c and don't print the
MHz if it's 0.  It's the indication that the data wasn't obtainable. I
think printing 0 is perhaps just as bad as printing the erroneous 100.

I trust you can make the adjustment without another round of patches.

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> index 9c9f362de..1c9ee5921 100644
> --- a/src/util/virhostcpu.c
> +++ b/src/util/virhostcpu.c
> @@ -590,12 +590,14 @@ virHostCPUParseFrequency(FILE *cpuinfo,
>      const char *prefix = NULL;
>      char line[1024];
>  
> +    /* No sensible way to retrieve CPU frequency */
> +    if (ARCH_IS_ARM(arch))
> +        return 0;
> +
>      if (ARCH_IS_X86(arch))
>          prefix = "cpu MHz";
>      else if (ARCH_IS_PPC(arch))
>          prefix = "clock";
> -    else if (ARCH_IS_ARM(arch))
> -        prefix = "BogoMIPS";
>  
>      if (!prefix) {
>          VIR_WARN("%s is not supported by the %s parser",
> diff --git a/tests/virhostcpudata/linux-aarch64-rhel74-moonshot.expected b/tests/virhostcpudata/linux-aarch64-rhel74-moonshot.expected
> index 24ff0ea0b..6776aa6c2 100644
> --- a/tests/virhostcpudata/linux-aarch64-rhel74-moonshot.expected
> +++ b/tests/virhostcpudata/linux-aarch64-rhel74-moonshot.expected
> @@ -1 +1 @@
> -CPUs: 8/8, MHz: 100, Nodes: 1, Sockets: 1, Cores: 8, Threads: 1
> +CPUs: 8/8, MHz: 0, Nodes: 1, Sockets: 1, Cores: 8, Threads: 1
> diff --git a/tests/virhostcpudata/linux-armv6l-raspberrypi.expected b/tests/virhostcpudata/linux-armv6l-raspberrypi.expected
> index 146bd073e..1c4c713d5 100644
> --- a/tests/virhostcpudata/linux-armv6l-raspberrypi.expected
> +++ b/tests/virhostcpudata/linux-armv6l-raspberrypi.expected
> @@ -1 +1 @@
> -CPUs: 1/1, MHz: 697, Nodes: 1, Sockets: 1, Cores: 1, Threads: 1
> +CPUs: 1/1, MHz: 0, Nodes: 1, Sockets: 1, Cores: 1, Threads: 1
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 5/5] util: Don't report CPU frequency for ARM hosts
Posted by Andrea Bolognani 7 years, 4 months ago 
On Fri, 2018-01-05 at 15:52 -0500, John Ferlan wrote:
> Suggestion - follow cmdNodeinfo for virhostcputest.c and don't print the
> MHz if it's 0.  It's the indication that the data wasn't obtainable. I
> think printing 0 is perhaps just as bad as printing the erroneous 100.
> 
> I trust you can make the adjustment without another round of patches.

Good idea. However, I've implemented it as a separate patch for a
few reasons:

* I've converted the test to use virBuffer, so the changes are not
  trivial enough that I feel comfortable pushing them without review;

* there are already a couple of files that contain 'MHz: 0' in the
  test suite, so it feels cleaner to change all of them at once;

* it's better to avoid performing more than one change per commit.

The follow-up patch is already on the list, in case you feel like
taking a look at it :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.