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

[opentitantool] Run "nice"r sims #16913

Closed
wants to merge 1 commit into from
Closed

[opentitantool] Run "nice"r sims #16913

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions sw/host/opentitanlib/src/transport/verilator/subprocess.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,10 @@ pub struct Subprocess {
impl Subprocess {
/// Starts a verilator [`Subprocess`] based on [`Options`].
pub fn from_options(options: Options) -> Result<Self> {
let mut command = Command::new(&options.executable);
let mut command = Command::new(String::from("nice"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't particularly like changing the command from the executable to 'nice', but it's the best way I know to deprioritize this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need the String::from? The docs for Command::new don't use it, but I didn't check the version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think "nice" is a "slice" and new needs a String, This is something I did to make OTT build happily.

Copy link
Contributor

Choose a reason for hiding this comment

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

I gave it a shot without wrapping in String::from and didn't have any trouble compiling. I admit I don't entirely understand the AsRef<OsStr> type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I guess there was some other issue that I've since fixed. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we able to use setpriority() or some other POSIX function instead of calling a separate program? I guess nice will probably be available anywhere those are, though, so maybe the function is not any better. 🤔

let mut args = Vec::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

Out-of-scope nit: Looks like Command::arg() and Command::args() exist. Given that command is already mutable, I think we could probably eliminate the args vector. Not super important and adds more noise to the diff, though.


args.push(String::from("-5"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Peeking at man nice, doesn't -5 need to be preceded by -n or --adjustment=?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes it clearer we're increasing niceness but less concise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, now I'm worried that nice might be reading plain -5 as a parameter and silently ignoring it.

% nice -5 bash -c 'echo hello'
hello
% nice -n -5 bash -c 'echo hello'
nice: cannot set niceness: Permission denied
hello

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works on my machine
nice -5 yes > /dev/null & htop

-5 should be equivalent to -n 5 IIRC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make it more verbose and readable, I just want to check that it works before I push the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice!

Copy link
Contributor

Choose a reason for hiding this comment

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

positive nicing requires lesser permissions

Good point. Positive nicing is less favorable to the process, so I think -5 is the wrong direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's misleading so I'm changing it
nice -5 nice:5
nice -n 5 nice:5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a different sort of hyphen

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! I thought it was silently ignoring -5, but it actually interpreted it as -n 5. Got it.

I think my persistent confusion about this is evidence it should be a verbose -n 5 :P

args.push(options.executable.to_string());
if !options.rom_image.is_empty() {
args.push(format!("--meminit=rom,{}", options.rom_image));
}
Expand Down