std: refactor `pthread`-based synchronization #128184

joboet · 2024-07-25T13:54:14Z

The non-trivial code for pthread_condvar is duplicated across the thread parking and the Mutex/Condvar implementations. This PR moves that code into sys::pal, which now exposes an unsafe wrapper type for pthread_mutex_t and pthread_condvar_t.

rustbot · 2024-07-25T13:54:22Z

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

bors · 2024-07-29T06:04:19Z

☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts.

ChrisDenton · 2024-07-30T14:30:59Z

This looks good to me on the surface but I don't feel I can confidently r+ atm as I don't currently have the time to go over it properly. So I'll reroll.

r? libs

workingjubilee

So, it seems like this is going to be a big diff either way. But is there any chance you can do the "coalesce in a single spot" and "change up various internals" in separate PRs? Or at least commits?

In particular, if possible, I'd like for the LazyBox -> OnceBox to be separately bisectable, even if it means I review somewhat gnarlier code.

bors · 2024-09-23T05:02:31Z

☔ The latest upstream changes (presumably #130724) made this pull request unmergeable. Please resolve the merge conflicts.

This PR replaces the `LazyBox` wrapper used to allocate the pthread primitives with `OnceBox`, which has a more familiar API mirroring that of `OnceLock`. This cleans up the code in preparation for larger changes like rust-lang#128184 (from which this PR was split) and allows some neat optimizations, like avoid an acquire-load of the allocation pointer in `Mutex::unlock`, where the initialization of the allocation must have already been observed. Additionally, I've gotten rid of the TEEOS `Condvar` code, it's just a duplicate of the pthread one anyway and I didn't want to repeat myself.

std: replace `LazyBox` with `OnceBox` This PR replaces the `LazyBox` wrapper used to allocate the pthread primitives with `OnceBox`, which has a more familiar API mirroring that of `OnceLock`. This cleans up the code in preparation for larger changes like rust-lang#128184 (from which this PR was split) and allows some neat optimizations, like avoid an acquire-load of the allocation pointer in `Mutex::unlock`, where the initialization of the allocation must have already been observed. Additionally, I've gotten rid of the TEEOS `Condvar` code, it's just a duplicate of the pthread one anyway and I didn't want to repeat myself.

Rollup merge of rust-lang#131094 - joboet:lazy_once_box, r=ibraheemdev std: replace `LazyBox` with `OnceBox` This PR replaces the `LazyBox` wrapper used to allocate the pthread primitives with `OnceBox`, which has a more familiar API mirroring that of `OnceLock`. This cleans up the code in preparation for larger changes like rust-lang#128184 (from which this PR was split) and allows some neat optimizations, like avoid an acquire-load of the allocation pointer in `Mutex::unlock`, where the initialization of the allocation must have already been observed. Additionally, I've gotten rid of the TEEOS `Condvar` code, it's just a duplicate of the pthread one anyway and I didn't want to repeat myself.

The non-trivial code for `pthread_condvar` is duplicated across the thread parking and the `Mutex`/`Condvar` implementations. This PR moves that code into `sys::pal`, which now exposes a non-movable wrapper type for `pthread_mutex_t` and `pthread_condvar_t`.

joboet · 2024-10-28T15:57:25Z

I've merged the OnceBox change in #131094, so this just contains the deduplication and wrapper creation.

@rustbot ready

workingjubilee · 2024-11-26T23:22:12Z

@bors try

bors · 2024-11-26T23:23:26Z

⌛ Trying commit 528b37a with merge a729c1c...

std: refactor `pthread`-based synchronization The non-trivial code for `pthread_condvar` is duplicated across the thread parking and the `Mutex`/`Condvar` implementations. This PR moves that code into `sys::pal`, which now exposes an `unsafe` wrapper type for `pthread_mutex_t` and `pthread_condvar_t`. try-job: dist-various-1 try-job: dist-various-2 try-job: test-various try-job: armhf-linux try-job: aarch64-gnu try-job: x86_64-fuchsia try-job: dist-i686-linux try-job: aarch64-apple try-job: dist-android try-job: dist-x86_64-freebsd

workingjubilee · 2024-11-26T23:35:35Z

@bors try

std: refactor `pthread`-based synchronization The non-trivial code for `pthread_condvar` is duplicated across the thread parking and the `Mutex`/`Condvar` implementations. This PR moves that code into `sys::pal`, which now exposes an `unsafe` wrapper type for `pthread_mutex_t` and `pthread_condvar_t`. try-job: dist-various-1 try-job: dist-various-2 try-job: test-various try-job: armhf-gnu try-job: aarch64-gnu try-job: x86_64-fuchsia try-job: dist-i686-linux try-job: aarch64-apple try-job: dist-android try-job: dist-x86_64-freebsd

bors · 2024-11-26T23:36:49Z

⌛ Trying commit 528b37a with merge a3a387a...

workingjubilee

Sorry about taking so long, it was kinda hard to follow all the moves.

We need to get more pedantic.

workingjubilee · 2024-11-27T01:07:08Z

library/std/src/sys/sync/condvar/pthread.rs

+        self.cvar.get_or_init(|| {
+            let mut cvar = Box::pin(pal::Condvar::new());
+            // SAFETY: we only call `init` once, namely here.
+            unsafe { cvar.as_mut().init() };
+            cvar
+        })


So, as I understand it, the closure can potentially get called twice, as the sync can race. That means that init has to be safe to call twice, even if it means that it can be "safe, but only if you immediately destroy it if it's a copy". That means that this // SAFETY comment is somewhat off-base.

The invariant is that init may only be called once per object; that isn't violated here.

I pried it apart.

I don't like "per object" as a constraint, because we have objects that manage objects, and we may create two objects to put inside one object, and then we have to get rid of one object, but we keep the other object, because we put it inside of the object...

... ... ...you get the picture.

What matters is that the pointee only sees one init.

library/std/src/sys/sync/condvar/pthread.rs

library/std/src/sys/sync/mutex/pthread.rs

bors · 2024-11-27T01:27:39Z

☀️ Try build successful - checks-actions
Build commit: a3a387a (a3a387ac353dfc5ddf9ccac50ea3a1982b1476b8)

workingjubilee · 2024-11-27T05:06:47Z

Passed on these jobs:

try-job: dist-various-1
try-job: dist-various-2
try-job: test-various
try-job: armhf-gnu
try-job: aarch64-gnu
try-job: x86_64-fuchsia
try-job: dist-i686-linux
try-job: aarch64-apple
try-job: dist-android
try-job: dist-x86_64-freebsd

workingjubilee · 2024-11-30T03:10:53Z

To put it another way:

Last I checked, the standard library has internal functions with "this actually, literally has to only be called once in the entire program" as a safety constraint.
This instead has a more loose sense of "once per mutex or condvar", and the actual usage is sound.
However, even saying "once per mutex or condvar" is ambiguous because we have sys::sync::Mutex, sys::pal::unix::sync::Mutex, and pthread_mutex_t. There's also "the mutex" as in "the thing that wraps the OnceBox" and there's "the mutex as in the thing we allocate and then put behind the OnceBox's pointer". And likewise when considering condvars.

Thinking again, my understanding is that what happens here, for both types, can be, hypothetically:

Thread A acquire-loads and finds the OnceBox of a sys::sync::Condvar or sys::sync::Mutex is empty.
Thread A allocates box_a, putting the pthread_mutex_t or pthread_condvar_t in the allocated space.
Thread A initializes the pthread_mutex_t or pthread_condvar_t in the allocated space owned by box_a.
Thread B performs the same steps as 1-3, creating box_b¹.
Finally, Thread A and Thread B both reach the AcqRel compare-exchange inside OnceBox, which forces them to sync. They play rock, paper, scissors over the memory bus for which gets to go first.
Today, Thread B wins, and writes box_b's pointer to the OnceBox field (.cvar or .pal).
Thread A sees that Thread B won, and has to run the destructors of the data it has created. So it will tear down the data allocated in the space of box_a if necessary, and then deallocate the pointer to this space. This is fine because nothing further could possibly have happened to the pthread_mutex_t or pthread_condvar_t after initialization, as we haven't exposed its address to anywhere except the pthread library, so it's not in a state where this is a concern.

As I understand it, this is sound, but we have objectively run the initialization function twice, and thus the safety comment is not really the clearest. We can't even say "once per condvar" or "once per mutex", because of how we have three different things we are calling a "mutex" or "condvar", so it's ambiguous, and "the thing we can only initialize once" is each place in memory. The job of this implementation is only to present the appearance that we've only allocated and initialized one pthread_mutex_t or pthread_condvar_t, not the reality that we have done so. My understanding is this is acceptable because we're calling functions that care about the allocated places in memory, behind the pointers we are operating on.

This is all fine. I just would like this to be clear from the comments, because we are three layers deep in terms of signifier vs. signified, we have indirections on top of indirections. The safety comments should be written more from the "insider perspective", since they're, well, inside the implementation.

Note that as long as both hit the acquire-load before they hit the next step, the compare-exchange, then neither are synchronized yet. ↩

workingjubilee

A possible diff that removes the ambiguity.

workingjubilee · 2024-11-30T03:28:25Z

library/std/src/sys/pal/unix/sync/mutex.rs

+    /// Must only be called once.
+    pub unsafe fn init(self: Pin<&mut Self>) {


Suggested change

/// Must only be called once.

pub unsafe fn init(self: Pin<&mut Self>) {

/// This value must only be initialized once.

pub unsafe fn init(self: Pin<&mut Self>) {

workingjubilee · 2024-11-30T03:29:09Z

library/std/src/sys/pal/unix/sync/condvar.rs

+    /// # Safety
+    /// May only be called once.


Suggested change

/// # Safety

/// May only be called once.

/// # Safety

/// This value must only be initialized once.

workingjubilee · 2024-11-30T03:29:40Z

library/std/src/sys/pal/unix/sync/condvar.rs

+    const CLOCK: libc::clockid_t = libc::CLOCK_MONOTONIC;
+
+    /// # Safety
+    /// May only be called once.


Suggested change

/// May only be called once.

/// This value must only be initialized once.

workingjubilee · 2024-11-30T03:33:47Z

library/std/src/sys/sync/condvar/pthread.rs

+    fn get(&self) -> Pin<&pal::Condvar> {
+        self.cvar.get_or_init(|| {
+            let mut cvar = Box::pin(pal::Condvar::new());
+            // SAFETY: we only call `init` once, namely here.


Suggested change

// SAFETY: we only call `init` once, namely here.

// SAFETY: pointee is initialized exactly once, by this closure

workingjubilee · 2024-11-30T03:34:56Z

library/std/src/sys/sync/mutex/pthread.rs

+        // This is sound however, as it cannot have been locked.
+        self.pal.get_or_init(|| {
+            let mut pal = Box::pin(pal::Mutex::new());
+            // SAFETY: we only call `init` once, namely here.


Suggested change

// SAFETY: we only call `init` once, namely here.

// SAFETY: pointee is initialized exactly once, by this closure

joboet · 2024-11-30T15:24:48Z

It's less about places in memory (you could safely destroy a pal::Mutex and initialize a new one in its place), but about instances of pal::Mutex/pal::Condvar. init may only be called once per instance of those. I've updated the comments to clarify this, hopefully that helps.

workingjubilee

thank you.

workingjubilee · 2024-12-01T00:00:46Z

@bors r+

bors · 2024-12-01T00:00:48Z

📌 Commit 8b2ff49 has been approved by workingjubilee

It is now in the queue for this repository.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#128184 (std: refactor `pthread`-based synchronization) - rust-lang#132047 (Robustify and genericize return-type-notation resolution in `resolve_bound_vars`) - rust-lang#133515 (fix: hurd build, stat64.st_fsid was renamed to st_dev) - rust-lang#133602 (fix: fix codeblocks in `PathBuf` example) - rust-lang#133622 (update link to "C++ Exceptions under the hood" blog) - rust-lang#133660 (Do not create trait object type if missing associated types) - rust-lang#133686 (Add diagnostic item for `std::ops::ControlFlow`) - rust-lang#133689 (Fixed typos by changing `happend` to `happened`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#128184 - joboet:refactor_pthread_sync, r=workingjubilee std: refactor `pthread`-based synchronization The non-trivial code for `pthread_condvar` is duplicated across the thread parking and the `Mutex`/`Condvar` implementations. This PR moves that code into `sys::pal`, which now exposes an `unsafe` wrapper type for `pthread_mutex_t` and `pthread_condvar_t`.

joboet added O-SGX Target: SGX O-unix Operating system: Unix-like A-atomic Area: Atomics, barriers, and sync primitives labels Jul 25, 2024

rustbot assigned ChrisDenton Jul 25, 2024

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 Jul 25, 2024

This comment has been minimized.

Sign in to view

joboet force-pushed the refactor_pthread_sync branch from 88c22d5 to 194cd19 Compare July 30, 2024 14:02

rustbot assigned workingjubilee and unassigned ChrisDenton Jul 30, 2024

This comment has been minimized.

Sign in to view

joboet force-pushed the refactor_pthread_sync branch 2 times, most recently from d9e06c4 to 1cf7f38 Compare July 30, 2024 14:48

workingjubilee requested changes Aug 13, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2024

joboet mentioned this pull request Oct 1, 2024

std: replace LazyBox with OnceBox #131094

Merged

joboet force-pushed the refactor_pthread_sync branch from 1cf7f38 to 528b37a Compare October 28, 2024 15:57

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 28, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 28, 2024

workingjubilee self-requested a review November 4, 2024 23:24

workingjubilee closed this Nov 26, 2024

workingjubilee reopened this Nov 26, 2024

This comment has been minimized.

Sign in to view

workingjubilee requested changes Nov 27, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 27, 2024

workingjubilee reviewed Nov 30, 2024

View reviewed changes

std: clarify comments about initialization

8b2ff49

workingjubilee approved these changes Dec 1, 2024

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 1, 2024

matthiaskrgr mentioned this pull request Dec 1, 2024

Rollup of 8 pull requests #133694

Merged

bors merged commit fe4c6e8 into rust-lang:master Dec 1, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Dec 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

std: refactor `pthread`-based synchronization #128184

std: refactor `pthread`-based synchronization #128184

joboet commented Jul 25, 2024 •

edited by workingjubilee

Loading

rustbot commented Jul 25, 2024

This comment has been minimized.

bors commented Jul 29, 2024

ChrisDenton commented Jul 30, 2024

This comment has been minimized.

workingjubilee left a comment

bors commented Sep 23, 2024

joboet commented Oct 28, 2024

workingjubilee commented Nov 26, 2024

bors commented Nov 26, 2024

This comment has been minimized.

workingjubilee commented Nov 26, 2024

bors commented Nov 26, 2024

workingjubilee left a comment

workingjubilee Nov 27, 2024

joboet Nov 27, 2024 •

edited

Loading

This comment was marked as outdated.

workingjubilee Nov 30, 2024

bors commented Nov 27, 2024

workingjubilee commented Nov 27, 2024

workingjubilee commented Nov 30, 2024 •

edited

Loading

workingjubilee left a comment

workingjubilee Nov 30, 2024

workingjubilee Nov 30, 2024

workingjubilee Nov 30, 2024

workingjubilee Nov 30, 2024

workingjubilee Nov 30, 2024

joboet commented Nov 30, 2024

workingjubilee left a comment

workingjubilee commented Dec 1, 2024

bors commented Dec 1, 2024

		/// Must only be called once.
		pub unsafe fn init(self: Pin<&mut Self>) {

	/// May only be called once.
	/// This value must only be initialized once.

	// SAFETY: we only call `init` once, namely here.
	// SAFETY: pointee is initialized exactly once, by this closure

std: refactor pthread-based synchronization #128184

std: refactor pthread-based synchronization #128184

Conversation

joboet commented Jul 25, 2024 • edited by workingjubilee Loading

rustbot commented Jul 25, 2024

This comment has been minimized.

bors commented Jul 29, 2024

ChrisDenton commented Jul 30, 2024

This comment has been minimized.

workingjubilee left a comment

Choose a reason for hiding this comment

bors commented Sep 23, 2024

joboet commented Oct 28, 2024

workingjubilee commented Nov 26, 2024

bors commented Nov 26, 2024

This comment has been minimized.

workingjubilee commented Nov 26, 2024

bors commented Nov 26, 2024

workingjubilee left a comment

Choose a reason for hiding this comment

workingjubilee Nov 27, 2024

Choose a reason for hiding this comment

joboet Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

workingjubilee Nov 30, 2024

Choose a reason for hiding this comment

bors commented Nov 27, 2024

workingjubilee commented Nov 27, 2024

workingjubilee commented Nov 30, 2024 • edited Loading

Footnotes

workingjubilee left a comment

Choose a reason for hiding this comment

workingjubilee Nov 30, 2024

Choose a reason for hiding this comment

workingjubilee Nov 30, 2024

Choose a reason for hiding this comment

workingjubilee Nov 30, 2024

Choose a reason for hiding this comment

workingjubilee Nov 30, 2024

Choose a reason for hiding this comment

workingjubilee Nov 30, 2024

Choose a reason for hiding this comment

joboet commented Nov 30, 2024

workingjubilee left a comment

Choose a reason for hiding this comment

workingjubilee commented Dec 1, 2024

bors commented Dec 1, 2024

std: refactor `pthread`-based synchronization #128184

std: refactor `pthread`-based synchronization #128184

joboet commented Jul 25, 2024 •

edited by workingjubilee

Loading

joboet Nov 27, 2024 •

edited

Loading

workingjubilee commented Nov 30, 2024 •

edited

Loading