Skip to content

implement cpuset_getaffinity for freebsd #4287

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
51 changes: 51 additions & 0 deletions src/shims/unix/freebsd/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,57 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
this.write_null(dest)?;
}

// The "same" kind of api as `sched_getaffinity` but more fine grained control for FreeBSD specifically.
"cpuset_getaffinity" => {
let [level, which, id, set_size, mask] =
this.check_shim(abi, Conv::C, link_name, args)?;

let level = this.read_scalar(level)?.to_i32()?;
let which = this.read_scalar(which)?.to_i32()?;
let id = this.read_scalar(id)?.to_i64()?;
let set_size = this.read_target_usize(set_size)?;
let mask = this.read_pointer(mask)?;

let _level_root = this.eval_libc_i32("CPU_LEVEL_ROOT");
let _level_cpuset = this.eval_libc_i32("CPU_LEVEL_CPUSET");
let level_which = this.eval_libc_i32("CPU_LEVEL_WHICH");

let _which_tid = this.eval_libc_i32("CPU_WHICH_TID");
let which_pid = this.eval_libc_i32("CPU_WHICH_PID");
let _which_jail = this.eval_libc_i32("CPU_WHICH_JAIL");
let _which_cpuset = this.eval_libc_i32("CPU_WHICH_CPUSET");
let _which_irq = this.eval_libc_i32("CPU_WHICH_IRQ");

// For sched_getaffinity, the current process is identified by -1.
// TODO: Use gettid? I'm not that familiar with this api.
let id = match id {
-1 => this.active_thread(),
_ =>
throw_unsup_format!(
"`cpuset_getaffinity` is only supported with a pid of -1 (indicating the current process)"
),
};

// We only support CPU_LEVEL_WHICH and CPU_WHICH_PID for now.
// This is the bare minimum to make the tests pass.
// TODO: Support more.
Comment on lines +136 to +137
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// This is the bare minimum to make the tests pass.
// TODO: Support more.
// This is the bare minimum to make the tests pass.

There's no point in having such a TODO in the code IMO. If we want to track this, we can have an issue.

if this.ptr_is_null(mask)? {
this.set_last_error_and_return(LibcError("EFAULT"), dest)?;
} else if level != level_which || which != which_pid {
throw_unsup_format!(
"`cpuset_getaffinity` is only supported with `level` set to CPU_LEVEL_WHICH and `which` set to CPU_WHICH_PID."
);
} else if let Some(cpuset) = this.machine.thread_cpu_affinity.get(&id) {
let cpuset = cpuset.clone();
let byte_count = set_size.try_into().unwrap();
this.write_bytes_ptr(mask, cpuset.as_slice()[..byte_count].iter().copied())?;
Copy link
Member

Choose a reason for hiding this comment

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

Won't this ICE Miri if byte_count is bigger than the mask?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes indeed, the changes are local, sorry.

this.write_null(dest)?;
} else {
// The thread whose ID is pid could not be found.
this.set_last_error_and_return(LibcError("ESRCH"), dest)?;
Comment on lines +150 to +151
Copy link
Member

Choose a reason for hiding this comment

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

This should be unreachable!(), right? The ID is always that of the active thread, after all.

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 was going by convention, but it should be unreachable.

Copy link
Member

Choose a reason for hiding this comment

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

The place where you copied this from likely supports other IDs, so it makes sense there, but it doesn't make sense to follow the same convention here.

}
}

_ => return interp_ok(EmulateItemResult::NotSupported),
}
interp_ok(EmulateItemResult::NeedsReturn)
Expand Down
4 changes: 2 additions & 2 deletions tests/pass/shims/available-parallelism-miri-num-cpus.rs
Copy link
Member

Choose a reason for hiding this comment

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

We should still test that 1024 works on platforms that support larger numbers of CPUs.

Copy link
Member

Choose a reason for hiding this comment

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

Please also add a test directly calling cpuset_getaffinity.

Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
//@compile-flags: -Zmiri-num-cpus=1024
//@compile-flags: -Zmiri-num-cpus=256

use std::num::NonZero;
use std::thread::available_parallelism;

fn main() {
assert_eq!(available_parallelism().unwrap(), NonZero::new(1024).unwrap());
assert_eq!(available_parallelism().unwrap(), NonZero::new(256).unwrap());
}