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

use_file: std::sync::Mutex, dropping all libpthread use. #457

Closed
wants to merge 2 commits into from

Conversation

briansmith
Copy link
Contributor

@briansmith briansmith commented Jun 5, 2024

use_file: std::sync::Mutex, dropping all libpthread use.

pthreads mutexes are not safe to move. While it is very unlikely that
the mutex we create will ever be moved, we don't actively do anything
to actively prevent it from being moved. (libstd, when it used/uses
pthreads mutexes, would box them to prevent them from being moved.)

Also, now on Linux and Android (and many other targets for which we
don't use use_std), libstd uses futexes instead of pthreads mutexes.
Thus using libstd's Mutex will be more efficient and avoid adding an
often-otherwise-unnecessary libpthreads dependency on these targets.

  • Linux, Android: Futex [1].
  • Haiku, Redox, NTO, AIX: pthreads [2].
  • others: not using use_file.

This will not affect our plans for --linux-none, since we don't
plan to use use_file for it. OnceLock

This breaks 32-bit x86 QNX Neutrino, which doesn't have libstd
because the target itself is abandoned [3]. the other QNX Neutrino
targets didn't get libstd support until Rust 1.69, so this
effectively raises the MSRV for them to 1.69.

Otherwise, the MSRV increases to 1.63 for the above-mentioned targets,
as that's when Mutex::new() became a const fn.

I tried to use Once to avoid the MSRV increase but it doesn't support
fallible initialization even in Nightly. OnceLock wasn't added until
1.70.

On x86_64 Linux, this change removes all libpthreads dependencies:

-       pthread_mutex_lock
-       pthread_mutex_unlock

and adds these libstd dependencies:

+       std::panicking::panic_count::GLOBAL_PANIC_COUNT
+       std::panicking::panic_count::is_zero_slow_path
+       std::sys::sync::mutex::futex::Mutex::lock_contended
+       std::sys::sync::mutex::futex::Mutex::wake

as measured using cargo asm.

[1] https://github.com/rust-lang/rust/blob/c1dba09f263cbff6170f130aa418e28bdf22bd96/library/std/src/sys/sync/mutex/mod.rs#L4-L10
[2] https://github.com/rust-lang/rust/blob/c1dba09f263cbff6170f130aa418e28bdf22bd96/library/std/src/sys/sync/mutex/mod.rs#L17-L20
[3] #453 (comment)

@briansmith briansmith force-pushed the b/std-mutex branch 3 times, most recently from 9a4d42a to bc8d06e Compare June 5, 2024 21:08
It is only used in one configuration, so put it with the other
stuff from that configuration.
@@ -31,3 +31,65 @@ fn is_getrandom_available() -> bool {
true
}
}

// Polls /dev/random to make sure it is ok to read from /dev/urandom.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As explained in the first commit, This code is only used on Linux/Android. With the other changes in this PR, DropGuard also became Linux/Android-specific. Instead of adding more #[cfg(...)] I just moved it all.

@josephlr
Copy link
Member

josephlr commented Jun 5, 2024

If we are adding a std dependency for these targets, should we also move to using std::File so that we don't even need DropGuard anymore? Also, should use of std require enabling the "std" cargo feature?

@briansmith
Copy link
Contributor Author

If we are adding a std dependency for these targets, should we also move to using std::File so that we don't even need DropGuard anymore?

Unless/until we can use std::syc::OnceLock or a polyfill for it, it doesn't make much sense right not to use std::File, because we'd end up using std::File::open() and then immediately calling into_raw_fd().

Also, should use of std require enabling the "std" cargo feature?

No. Note that this PR effectively only uses libstd on targets for which we know libstd is already available.

@briansmith
Copy link
Contributor Author

Unless/until we can use std::syc::OnceLock or a polyfill for it, it doesn't make much sense right not to use std::File, because we'd end up using std::File::open() and then immediately calling into_raw_fd().

Well, actually, it is possible to use std::File in wait_until_rng_ready now.

@briansmith
Copy link
Contributor Author

In #458, instead of moving wait_until_rng_ready, I took @josephlr suggestion and replaced DropGuard with std::fs::File.

pthreads mutexes are not safe to move. While it is very unlikely that
the mutex we create will ever be moved, we don't actively do anything
to actively prevent it from being moved. (libstd, when it used/uses
pthreads mutexes, would box them to prevent them from being moved.)

Also, now on Linux and Android (and many other targets for which we
don't use use_std), libstd uses futexes instead of pthreads mutexes.
Thus using libstd's Mutex will be more efficient and avoid adding an
often-otherwise-unnecessary libpthreads dependency on these targets.

  * Linux, Android: Futex [1].
  * Haiku, Redox, NTO, AIX: pthreads [2].
  * others: not using `use_file`.

This will not affect our plans for *-*-linux-none, since we don't
plan to use `use_file` for it. OnceLock

This breaks 32-bit x86 QNX Neutrino, which doesn't have libstd
because the target itself is abandoned [3]. the other QNX Neutrino
targets didn't get libstd support until Rust 1.69, so this
effectively raises the MSRV for them to 1.69.

Otherwise, the MSRV increases to 1.63 for the above-mentioned targets,
as that's when `Mutex::new()` became a `const fn`.

I tried to use `Once` to avoid the MSRV increase but it doesn't support
fallible initialization even in Nightly. `OnceLock` wasn't added until
1.70.

On x86_64 Linux, this change removes all libpthreads dependencies:

```diff
-       pthread_mutex_lock
-       pthread_mutex_unlock
```

and adds these libstd dependencies:

```diff
+       std::panicking::panic_count::GLOBAL_PANIC_COUNT
+       std::panicking::panic_count::is_zero_slow_path
+       std::sys::sync::mutex::futex::Mutex::lock_contended
+       std::sys::sync::mutex::futex::Mutex::wake
```

as measured using `cargo asm`.

[1] https://github.com/rust-lang/rust/blob/c1dba09f263cbff6170f130aa418e28bdf22bd96/library/std/src/sys/sync/mutex/mod.rs#L4-L10
[2] https://github.com/rust-lang/rust/blob/c1dba09f263cbff6170f130aa418e28bdf22bd96/library/std/src/sys/sync/mutex/mod.rs#L17-L20
[3] rust-random#453 (comment)
@briansmith
Copy link
Contributor Author

Closing this in favor of #458 based on @newpavlov's comment in the issue.

@briansmith briansmith closed this Jun 5, 2024
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