MSRV 1.63: Use libstd instead of libc::open and instead of libpthreads's Mutex.
#458
Conversation
briansmith
force-pushed
the
b/std-1
branch
2 times, most recently
from
b2d64b1
to
1ba3a1c
Compare
| unsafe { MUTEX.lock() }; | ||
| let _guard = DropGuard(|| unsafe { MUTEX.unlock() }); | ||
| fn get_fd_locked() -> Result<RawFd, Error> { | ||
| static MUTEX: Mutex<()> = Mutex::new(()); |
There was a problem hiding this comment.
Another possibility, ideally deferred to a future PR as it is riskier, would be to replace the double-checked locking with a simple RwLock<File>, on the assumption that acquiring the read lock is fast enough that avoiding doing it isn't necessary.
There was a problem hiding this comment.
Actually, maybe that can't be deferred. I was assuming that a locking a mutex has Ordering::Acquire semantics and unlocking it would have Ordering::Release semantics. However, Mutex doesn't guarantee Acquire/Release semantics at all; presumably it does have them for things inside the mutex, but FD isn't in the Mutex, only a () is. A Mutex<()> could theoretically have all its synchronization optimized away completely.
There was a problem hiding this comment.
As I mentioned elsewhere, I filed rust-lang/rust#126239 to get libstd to document the widely-assumed "fact" that Mutxes have acquire/release semantis.
briansmith
changed the title
libstd instead of libc::open and instead of libpthreads's Mutex.libstd instead of libc::open and instead of libpthreads's Mutex.
| unsafe { MUTEX.lock() }; | ||
| let _guard = DropGuard(|| unsafe { MUTEX.unlock() }); | ||
| fn get_fd_locked() -> Result<RawFd, Error> { | ||
| static MUTEX: Mutex<()> = Mutex::new(()); |
There was a problem hiding this comment.
Actually, maybe that can't be deferred. I was assuming that a locking a mutex has Ordering::Acquire semantics and unlocking it would have Ordering::Release semantics. However, Mutex doesn't guarantee Acquire/Release semantics at all; presumably it does have them for things inside the mutex, but FD isn't in the Mutex, only a () is. A Mutex<()> could theoretically have all its synchronization optimized away completely.
src/use_file.rs
Outdated
| FD.store(fd as usize, Relaxed); | ||
| debug_assert!(fd >= 0); | ||
| const _: () = assert!(FD_UNINIT < 0); | ||
| FD.store(fd, Relaxed); |
There was a problem hiding this comment.
It isn't strictly guaranteed that this FD.store() is going to happen before the mutex is unlocked. It seems like here we'd at least benefit from a fence, something like this:
// Synchronization is handled be `fence()` immediately after.
FD.store(fd, Ordering::Relaxed);
// Release: Sync with `FD.load(Ordering::Acquire)`;
// Acquire: Force `FD` to be writen before we release the lock.
// `Mutex` doesn't guarantee it will wait for the above store
// before unlocking, because `FD` isn't within the Mutex.
core::sync::atomic::fence(Ordering::AcqRelease);
Although, again, it isn't clear that Mutex<()> actually is guaranteed to do anything meaningful.
briansmith
force-pushed
the
b/std-1
branch
8 times, most recently
from
2ed5708
to
9dae12f
Compare
For now, still use `libc::{poll,read}`. But use `File::open` to open
the files, instead of using `DropGuard`.
While doing this, switch to the `RawFd` type alias from `libc::c_int`.
briansmith
force-pushed
the
b/std-1
branch
3 times, most recently
from
b02470b
to
e348fa8
Compare
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. 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. I tried to use `Once` to avoid the MSRV increase but it doesn't support fallible initialization. `OnceLock` wasn't added until 1.70 but even then, `OnceLock::get_or_try_init` is still unstable. 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)
|
The current version of the crate in the master branch no longer relies libpthreads's |
See each individual commit message for full details.