Skip to content

Conversation

@patrickelectric
Copy link
Contributor

Signed-off-by: Patrick José Pereira patrickelectric@gmail.com

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
@patrickelectric patrickelectric force-pushed the governor branch 5 times, most recently from 3231833 to 31dce84 Compare September 3, 2023 16:11
@patrickelectric patrickelectric marked this pull request as draft September 3, 2023 16:19
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
…mExt

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
@patrickelectric patrickelectric marked this pull request as ready for review September 3, 2023 16:52
@patrickelectric
Copy link
Contributor Author

@GuillaumeGomez it's ready to review, it appears that the CI error is not from this patch

sys.global_cpu_info().frequency()
);
}
"governor" => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not displaying this information as part of the CPUs directly?

/// );
/// println!("{}%", s.global_cpu_info().cpu_usage());
/// ```
fn governor(&self) -> GovernorKind;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this from SystemExt, it should only be on CpuExt.

/// println!("{}", cpu.governor());
/// }
/// ```
fn governor(&self) -> GovernorKind;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should return an Option in case this information hasn't been retrieved yet.

@GuillaumeGomez
Copy link
Owner

A few things to change but also: I won't merge this PR until this feature is implemented for all supported systems.

@patrickelectric
Copy link
Contributor Author

A few things to change but also: I won't merge this PR until this feature is implemented for all supported systems.

Is this truly necessary ? Can't we make it an optional and have others OSs implementing it and allowing not having support ?

I don't have access to all OSs, and that's open source.. others may add this functionality if it's necessary, I'm just creating the background to support it.

@GuillaumeGomez
Copy link
Owner

Is this truly necessary ?

Yes it is.

Can't we make it an optional and have others OSs implementing it and allowing not having support ?

The only way to have something optional is in case an OS doesn't support this feature. Otherwise it might very well never be implemented. It also forces to add special handling in tests, meaning more code to maintain.

I don't have access to all OSs, and that's open source.. others may add this functionality if it's necessary, I'm just creating the background to support it.

I do everything with VMs. You can always ask help and see if others are interested into this feature and willing to help you implement it.

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.

2 participants