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.

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

sysinfo: Add new frequency detection method #701

Merged
merged 2 commits into from
Jul 16, 2024

Conversation

n0toose
Copy link
Member

@n0toose n0toose commented Jun 5, 2024

This is a proof-of-concept, there is some more testing necessary and some questions as to how this feature should be implemented: #690

TODOs

  • Does this work on aarch64 (Raspberry Pi)?
  • Write test.
  • How do we deal with sysinfo on x86_64? (Do we use sysinfo instead of CPUID, or should it be exclusively an aarch64 thing? If yes, do we throw out the CPUID code?)
  • Investigate potential conflict with fix(x86_64): handle processor frequency not present in CPUID #688
  • Remove TODO comments
  • Fix formatting.
  • FrequencyDetectionFailed duplication?
  • Better design, so that we don't have to convert u64 into u32?
    • Possibly negligible, the current test suite does not expect frequencies over 10Ghz.

I don't think that there's a "serious" case where it would return a zero, so this would technically turn the alternatives (e.g. CPUID) into dead code 99.99% of the time.

Nice-to-haves

  • The sysinfo crate adds a new dependency on windows crates. Perhaps a feature upstream that would remove that dependency would be great, as Windows is not a target.

  • 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: #690

@n0toose n0toose marked this pull request as draft June 5, 2024 11:38
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 71.73913% with 13 lines in your changes missing coverage. Please review.

Project coverage is 67.03%. Comparing base (f8b1426) to head (11f5a44).

Files Patch % Lines
src/vm.rs 71.42% 12 Missing ⚠️
src/arch/mod.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #701      +/-   ##
==========================================
- Coverage   68.17%   67.03%   -1.15%     
==========================================
  Files          20       21       +1     
  Lines        2319     2357      +38     
==========================================
- Hits         1581     1580       -1     
- Misses        738      777      +39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@n0toose
Copy link
Member Author

n0toose commented Jun 5, 2024

Oops, test fails because I forgot to move the proof-of-concept sysinfo function into the aarch64 variant. It should work locally on x86_64.

@n0toose
Copy link
Member Author

n0toose commented Jun 7, 2024

I'm not sure if "Draft" PRs in the context of hermit-os are interpreted as "Please don't look at this / review this PR (yet)" or "This needs some extra work before it can be merged" (as I've seen both in the context of OSS projects before). In my case, this PR needs some extra work and could benefit from feedback. I have the following question before finalizing this PR:

How do we deal with sysinfo on x86_64? (Do we use sysinfo instead of CPUID, or should it be exclusively an aarch64 thing? If yes, do we throw out the CPUID code?)

This is not urgent or a call for attention - just a clarification of my intentions and why the PR has stalled. I'll work on something else in the meantime :)

@mkroening mkroening requested review from jounathaen and mkroening and removed request for jounathaen June 7, 2024 18:16
src/arch/mod.rs Outdated Show resolved Hide resolved
src/arch/x86_64/mod.rs Outdated Show resolved Hide resolved
@jounathaen
Copy link
Member

  • Does this work on aarch64 (Raspberry Pi)?

I assume it does. Looks like your approach is fine.

  • How do we deal with sysinfo on x86_64? (Do we use sysinfo instead of CPUID, or should it be exclusively an aarch64 thing? If yes, do we throw out the CPUID code?)

I like the current approach you've taken

I don't see a conflict here.

  • FrequencyDetectionFailed duplication?

True. You should create an error outside of the arch mods and import it in arch::x86...

  • Better design, so that we don't have to convert u64 into u32?
  • Possibly negligible, the current test suite does not expect frequencies over

As written above, this is fine as is

src/arch/mod.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@n0toose
Copy link
Member Author

n0toose commented Jul 2, 2024

Apparently, the example of "would it brake on macos-latest" was not a hypothetical - the test would fail in that case, because the frequency value is not OK (see action). This should have been fixed here and perhaps warrants some further investigation before merging: GuillaumeGomez/sysinfo#1023

#701 (comment)

@n0toose n0toose changed the title WIP: sysinfo: Add new frequency detection method sysinfo: Add new frequency detection method Jul 2, 2024
@n0toose n0toose marked this pull request as ready for review July 2, 2024 15:55
n0toose and others added 2 commits July 16, 2024 14:26
- 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
@jounathaen jounathaen added this pull request to the merge queue Jul 16, 2024
Merged via the queue into hermit-os:main with commit a0586d4 Jul 16, 2024
9 of 10 checks passed
@jounathaen jounathaen deleted the sysinfo-dev branch July 16, 2024 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cross-platform CPU frequencies
2 participants