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

Conversation

LorrensP-2158466
Copy link
Contributor

@LorrensP-2158466 LorrensP-2158466 commented Apr 22, 2025

Fixes #4265.

This implements the FreeBSD syscall cpuset_getaffinity. In practice, this syscall behaves like a more fine-grained api than sched_getaffinity.

The tests only require one specific combination of parameters to pass, so this is all that is implemented. This can be done in future work (or now if needed).

However, I had to change 1 test file available-parallelism-miri-num-cpus. This test sets the number of CPUs to 1024, but the libc API is structured so that only 256 can be accepted. (I'll explain in a comment.)

I have not changed the ci script since I don't know what I should put there.

@LorrensP-2158466
Copy link
Contributor Author

So the tests use the libc function libc::CPU_COUNT, which counts how many bits are flipped to 1 in the cpuset. The source code that calls the syscall uses the struct libc::cpuset_t, and the size of this set struct is used to write the specified bytes to the mask pointer.

The source of this struct is this:

pub struct cpuset_t {
    #[cfg(all(any(freebsd15, freebsd14), target_pointer_width = "64"))]
    __bits: [c_long; 16],
    #[cfg(all(any(freebsd15, freebsd14), target_pointer_width = "32"))]
    __bits: [c_long; 32],
    #[cfg(all(not(any(freebsd15, freebsd14)), target_pointer_width = "64"))]
    __bits: [c_long; 4],
    #[cfg(all(not(any(freebsd15, freebsd14)), target_pointer_width = "32"))]
    __bits: [c_long; 8],
}

So on newer FreeBSD version, this struct is 128 bytes large, otherwise 32 bytes. During testing I have found out that the current compilation results in the 32 bytes variant. This means we can only "use" 256 CPUs since the size of the mask is: 32 * 8 = 256. Miri's maximum number is 1024 and the test also sets and expects this value.

I have no idea how to alter compilation to use the 128 size variant, which would result in 1024 bits being used. But the test passes when using -Zmiri-num-cpus=256.

@LorrensP-2158466
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Apr 22, 2025
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.

@RalfJung
Copy link
Member

RalfJung commented Apr 23, 2025

I have no idea how to alter compilation to use the 128 size variant,

The standard library is built for compatibility with FreeBSD 12, so we'd have to wait until that gets bumped to FreeBSD 14.

What happens on FreeBSD when setting num-cpus too high? We should at least fail somewhat gracefully.

@LorrensP-2158466
Copy link
Contributor Author

The user mask is only 32 bytes, so it would only write 32 bytes of the mask (which is 128). You can try and write 128 bytes, but then miri catches a buffer overflow.

@RalfJung
Copy link
Member

So set_size would be 32 bytes and we'd just not write the rest, truncating as usual? But no UB?

I'd say make that test have two revisions, one for 256 cores and one for 1024 cores. The 256 revision should always give the right reply, and the 1024 revision will then presumably just return 256 on FreeBSD.

@LorrensP-2158466
Copy link
Contributor Author

So set_size would be 32 bytes and we'd not write the rest, truncating as usual? But no UB?

Nothing is truncated per se, but the full mask that Miri holds can't be written to the user-provided mask, writing more causes UB:

   |
LL |                       if libc::cpuset_getaffinity(
   |  ________________________^
LL | |                         libc::CPU_LEVEL_WHICH,
LL | |                         libc::CPU_WHICH_PID,
LL | |                         -1,
LL | |                         size_of::<libc::cpuset_t>(),
LL | |                         &mut set,
LL | |                     ) == 0 {
   | |_____________________^ memory access failed: expected a pointer to 128 bytes of memory, but got alloc994 which is only 32 bytes from the end of the allocation

I'd say make that test have two revisions

Revision as in 2 test files?

@RalfJung
Copy link
Member

Revision as in 2 test files?

No, as in one test file with 2 revisions. Grep the test suite for revisions: if you have not seen this yet.

Nothing is truncated per se, but the full mask that Miri holds can't be written to the user-provided mask, writing more causes UB:

Wait, so calling this safe standard library method causes UB? That's bad. It's either a bug in Miri or a bug in the standard library.

@LorrensP-2158466
Copy link
Contributor Author

Alright thanks!

Wait, so calling this safe standard library method causes UB? That's bad. It's either a bug in Miri or a bug in the standard library.

No, hahaha, I'm explaining this very badly. The implementation handles this correctly, either writing set_size bytes into the user mask, or writing the entire Mask held in Miri. It's just when I was implementing this I did cause a bug in Miri by writing the full Mask of Miri into the mask of the user. I did this because all those bits are needed for the test.

The correct thing to do here is this (in pseudo-code):

let byte_count = Ord::min(set_size, miri_cpu_set_of_thread.byte_len());
// copy byte_count amount of bytes from miri_cpu_set_of_thread into user_mask

Writing the full mask held in Miri would cause UB, because the user mask in older FreeBSD versions is much smaller.

Sorry for the bad explanation :(.

@RalfJung
Copy link
Member

Yeah, Miri needs to clamp somewhere, agreed. That's exactly why I want to see a test of "mask too small". :)

tests/pass-dep/libc/libc-affinity.rs has such a test for sched_getaffinity; we'll want similar tests directly calling cpuset_getaffinity.

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

@rustbot author

Comment on lines +136 to +137
// This is the bare minimum to make the tests pass.
// TODO: Support more.
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.

Comment on lines +150 to +151
// The thread whose ID is pid could not be found.
this.set_last_error_and_return(LibcError("ESRCH"), dest)?;
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.

} 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.

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.

@rustbot rustbot removed the S-waiting-on-review Status: Waiting for a review to complete label Apr 29, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: Waiting for the PR author to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support num_cpus on FreeBSD
3 participants