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

std: available_parallelism using native netbsd api first #112226

Merged
merged 1 commit into from
Jun 17, 2023

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented Jun 2, 2023

before falling back to existing code paths like FreeBSD does.

@rustbot
Copy link
Collaborator

rustbot commented Jun 2, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 2, 2023
Comment on lines 348 to 368
#[cfg(target_os = "netbsd")]
{
unsafe {
let set = libc::_cpuset_create();
if !set.is_null() {
let mut count: usize = 0;
if libc::pthread_getaffinity_np(libc::pthread_self(), libc::_cpuset_size(set), set) == 0 {
#[cfg(target_pointer_width = "32")]
const MAXCPUS: u64 = 32;
#[cfg(target_pointer_width = "64")]
const MAXCPUS: u64 = 256;
for i in 0..MAXCPUS {
match libc::_cpuset_isset(i, set) {
-1 => break,
0 => continue,
_ => count = count + 1,
}
}
}
libc::_cpuset_destroy(set);
if count > 0 {
return Ok(NonZeroUsize::new_unchecked(count));
}
}
}
}
Copy link
Member

@workingjubilee workingjubilee Jun 3, 2023

Choose a reason for hiding this comment

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

Since this doesn't need to affect the rest of the code until it hits a final return, can the majority of this block be moved out-of-line into a Rust function so the in-line code is something like if let Some(x) = netbsd_available_parallelism() { return Ok(x) };?

Copy link
Member

@workingjubilee workingjubilee Jun 3, 2023

Choose a reason for hiding this comment

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

I don't know, maybe that's actually harder to read/follow? The platform grouping just feels odd with two mutually-exclusive cfg blocks inside what is already a cfg-if branch. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since these two blocks are not gonna be reusable pieces, would not make much sense to make them as separate calls.

Copy link
Member

@workingjubilee workingjubilee Jun 3, 2023

Choose a reason for hiding this comment

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

Well, there's plenty of other called-once fn in std, they're just usually used as an abstraction boundary (so you don't have to look at both if only one is relevant). I'm not married to that idea, though, I'm mostly just wondering because it is getting... well, has been for some time a bit hard to read std::sys in general, so I'm wondering what directions might be good for improvement.

Copy link
Member

Choose a reason for hiding this comment

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

The linux cgroup stuff has been extracted into its own module for example.

const MAXCPUS: u64 = 32;
#[cfg(target_pointer_width = "64")]
const MAXCPUS: u64 = 256;
for i in 0..MAXCPUS {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wary of hard-coding MAXCPUS. Would it be sufficient to just count unbounded (or to u64::MAX) and let -1 => break do its job?

Comment on lines 368 to 369
if count > 0 {
return Ok(NonZeroUsize::new_unchecked(count));
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
if count > 0 {
return Ok(NonZeroUsize::new_unchecked(count));
if let Some(count) = NonZeroUsize::new(count) {
return Ok(count);

before falling back to existing code paths like FreeBSD does.
@cuviper
Copy link
Member

cuviper commented Jun 16, 2023

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 16, 2023

📌 Commit 25b3751 has been approved by cuviper

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 16, 2023
…mpiler-errors

Rollup of 7 pull requests

Successful merges:

 - rust-lang#111074 (Relax implicit `T: Sized` bounds on `BufReader<T>`, `BufWriter<T>` and `LineWriter<T>`)
 - rust-lang#112226 (std: available_parallelism using native netbsd api first)
 - rust-lang#112474 (Support 128-bit enum variant in debuginfo codegen)
 - rust-lang#112662 (`#[lang_item]` for `core::ptr::Unique`)
 - rust-lang#112665 (Make assumption functions in new solver take `Binder<'tcx, Clause<'tcx>>`)
 - rust-lang#112684 (Disable alignment checks on i686-pc-windows-msvc)
 - rust-lang#112706 (Add `SyntaxContext::is_root`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4d5e7cd into rust-lang:master Jun 17, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 17, 2023
he32 added a commit to he32/rust that referenced this pull request Oct 12, 2023
…ism".

First off, I have it on good authority this code is Just Wrong: by
default a given thread does not have affinity to any specific set
of CPUs.

This particular change came with this pull request:

  rust-lang#112226

However, even worse, this code causes a segmentation fault for certain
NetBSD target architectures in the "bootstrap" program when building
rust natively on those platforms.  So far armv7/9.0, powerpc/10.0_BETA,
and i386/9.3 all crash with a segmentation fault.  However, for some
strange reason, this isn't consistent across the board: riscv64/current,
amd64/10.0_BETA, aarch64/9.0 and sparc64/10.0_BETA all pass this
hurdle.  A trivial C reimplementation also doesn't crash on any of
these systems, ref. the thread which starts at

  https://mail-index.netbsd.org/current-users/2023/10/10/msg044510.html

but also always prints 0.  However, if we get a SEGV running this
code, the entire build fails, of course.

So ... while I do not have a full explanation for the SEGVs, this
undoes the addition from pull request 112226, and restores the ability
to build rust natively on the above flagged-as-problematical platforms.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants