Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cross-platform CPU frequencies #690

Closed
mkroening opened this issue May 7, 2024 · 4 comments · Fixed by #701
Closed

Cross-platform CPU frequencies #690

mkroening opened this issue May 7, 2024 · 4 comments · Fixed by #701
Assignees
@n0toose
Labels
good first issue Good for newcomers

Comments

@mkroening
Copy link
Member

We might want to explore sysinfo for getting CPU frequencies from the OS instead of parsing CPUID (x86 only) ourselves. See hermit-os/kernel/xtask/src/ci/qemu.rs#L283-L291 for example.
@mkroening mkroening added the good first issue Good for newcomers label May 7, 2024
@mkroening mkroening mentioned this issue May 7, 2024
Closed
@n0toose
Copy link
Member

n0toose commented May 19, 2024

Hi, I'm just looking around and have one quick question: The result from sysinfo is much more "accurate" (as in, as accurate as cat /proc/cpuinfo) than the result provided by CPUID.

Given that the fallback option - if all else fails - is guessing a specific frequency of around 2 GHz (when testing rusty_demo), and that we only require one core to get that number anyway, the number that we disclose to the application and the way that we obtained that number is, practically speaking, irrelevant, correct?

P.S. Feel free to assign this issue to me, I'll submit a PR in June.

@n0toose
Copy link
Member

n0toose commented May 19, 2024 •
edited
Loading

We might want to explore sysinfo for getting CPU frequencies from the OS instead of parsing CPUID (x86 only)

Oh, and one more thing: As I'm not sure what the meaning of that value is, I am also not 100% sure if sysinfo would be a sane default for x86 and whether it should replace detect_freq_from_cpuid and detect_freq_from_cpuid_hypervisor_info entirely?

@mkroening
Copy link
Member Author

As I'm not sure what the meaning of that value is, I am also not 100% sure if sysinfo would be a sane default for x86 and whether it should replace detect_freq_from_cpuid and detect_freq_from_cpuid_hypervisor_info entirely?

Eventually, that number ends up in the kernel (src/arch/x86_64/kernel/processor.rs#L99-L105), where different strategies (src/arch/x86_64/kernel/processor.rs#L478-L496) are used for determining the processor frequency. As far as I know, this is mostly used for determining time durations via the CPU timestamp, which is CPU frequency dependent. So the more accurate the resulting time duration is, the better.

The result from sysinfo is much more "accurate" (as in, as accurate as cat /proc/cpuinfo) than the result provided by CPUID.

I am not sure which one is better. I don't know if sysinfo provides the real current frequency that is subject to dynamic frequency changes and whether that corresponds to the frequency of the Time Stamp Counter (TSC), that we use. The last fallback in the kernel is to measure the frequency via the Programmable Interval Timer (PIT). Maybe that provides better values? In that case, we could just provide None as frequency and let the kernel measure it.

@n0toose
Copy link
Member

n0toose commented May 21, 2024 •
edited
Loading

I am not sure which one is better. I don't know if sysinfo provides the real current frequency that is subject to dynamic frequency changes

It does. I did a rough test by changing my laptop from "Power Saving Mode" to "Performance Mode" and could see the difference in a PoC (hence the /proc/cpuinfo mention) - I believe that it is assumed that this value will not change from one second to another. I think that it might be best to wait until my PR, as an apt proof-of-concept could help figure this one out. I'll take a look at how the kernel measures the frequency in the meantime (and also consider what you just wrote).

Based on

uhyve/src/vm.rs

Line 216 in 2afc95f

cpu_freq: NonZeroU32::new(detect_cpu_freq() * 1000),
, I think it is safe to assume that the software does not assume that the value can be "changed" during execution, which is reasonable, given that a CPUID-parsed value would be parsed instead.

n0toose added a commit to n0toose/uhyve that referenced this issue Jun 5, 2024
@n0toose
WIP: sysinfo: Add new frequency detection method
9ae4a05 
- This is a work-in-progress.
  * This change has not been tested on aarch64, for which it is
    primarily intended for. (See hermit-os#690)
  * sysinfo returns a u64 value, which we then try to fit into u32
    (because the alternative methods of getting a frequency value return
    a u32), this may not be clean.
  * The frequency is not all that useful (after all, if we pass a value
    of 0, running `rusty_demo` will result in a "guessed" frequency), but
    sysinfo returns a base frequency that has a completely different "meaning"
    than the value returned by CPUID on x86_64.
  * Do we use sysinfo by default on x86_64 as well? The alternative methods of
    obtaining the frequency would be essentially turned into "dead code".

- Add detect_freq_from_sysinfo()

  Shows a real-time value of the base frequency, similarly to
  /proc/cpuinfo on Linux.

  Using sysinfo as an alternative to the CPUID-provided value may be useful
  for some systems where the CPUID feature is not available. A potential
  flaw of this implementation is that the frequency is passed once to
  before the initialization of the kernel and cannot be changed later.

  If the host system is in "power-saving mode" (e.g. in laptops) and then
  switches to "performance mode", the application will not have any
  knowledge of this change. IIRC, Jonathan thinks this is OK for now.

- Use sysinfo by default on x86_64 instead of CPUID.

  Currently for demonstration reasons, we may want to amend this in a
  later revision.

Fixes: hermit-os#690
@n0toose n0toose mentioned this issue Jun 5, 2024
Merged
9 tasks
n0toose added a commit to n0toose/uhyve that referenced this issue Jul 8, 2024
@n0toose
WIP: sysinfo: Add new frequency detection method
c55adbc 
- This is a work-in-progress.
  * This change has not been tested on aarch64, for which it is
    primarily intended for. (See hermit-os#690)
  * sysinfo returns a u64 value, which we then try to fit into u32
    (because the alternative methods of getting a frequency value return
    a u32), this may not be clean.
  * The frequency is not all that useful (after all, if we pass a value
    of 0, running `rusty_demo` will result in a "guessed" frequency), but
    sysinfo returns a base frequency that has a completely different "meaning"
    than the value returned by CPUID on x86_64.
  * Do we use sysinfo by default on x86_64 as well? The alternative methods of
    obtaining the frequency would be essentially turned into "dead code".

- Add detect_freq_from_sysinfo()

  Shows a real-time value of the base frequency, similarly to
  /proc/cpuinfo on Linux.

  Using sysinfo as an alternative to the CPUID-provided value may be useful
  for some systems where the CPUID feature is not available. A potential
  flaw of this implementation is that the frequency is passed once to
  before the initialization of the kernel and cannot be changed later.

  If the host system is in "power-saving mode" (e.g. in laptops) and then
  switches to "performance mode", the application will not have any
  knowledge of this change. IIRC, Jonathan thinks this is OK for now.

- Use sysinfo by default on x86_64 instead of CPUID.

  Currently for demonstration reasons, we may want to amend this in a
  later revision.

Fixes: hermit-os#690
jounathaen pushed a commit to n0toose/uhyve that referenced this issue Jul 16, 2024
@n0toose @jounathaen
Add new frequency detection method based on sysinfo
0bb6352 
- Add detect_freq_from_sysinfo()

  Shows a real-time value of the base frequency, similarly to
  /proc/cpuinfo on Linux.

  Using sysinfo as an alternative to the CPUID-provided value may be useful
  for some systems where the CPUID feature is not available. A potential
  flaw of this implementation is that the frequency is passed once to
  before the initialization of the kernel and cannot be changed later.

  If the host system is in "power-saving mode" (e.g. in laptops) and then
  switches to "performance mode", the application will not have any
  knowledge of this change. IIRC, Jonathan thinks this is OK for now.

- Use sysinfo by default on x86_64 instead of CPUID.

Fixes: hermit-os#690
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@n0toose n0toose

Labels
good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sysinfo: Add new frequency detection method n0toose/uhyve
2 participants
@mkroening @n0toose