std: use a queue-based Condvar on NetBSD and other platforms
#127578
Conversation
rustbot
added
O-windows
|
This is OS level stuff that I'm not familiar with. r? libs |
| //! thread handle is removed from the list. | ||
| //! | ||
| //! The list itself has the same structure as the one used by the queue-based | ||
| //! `RwLock` implementation, see its documentation for more information. This |
There was a problem hiding this comment.
Is there any way to share the code for that queue, rather than having duplicate code with the same structure?
There was a problem hiding this comment.
Yes, but it would be a bit complicated, since the queue-based RwLock is used on more platforms. Also, since they aren't accessed concurrently the nodes need atomic pointer here, but for RwLock that's not the case (there can be multiple reader threads that traverse the queue).
|
Rerolling because I don't think I'll have time for this soon. r? libs |
|
☔ The latest upstream changes (presumably #127819) made this pull request unmergeable. Please resolve the merge conflicts. |
workingjubilee
added
O-netbsd
| //! | ||
| //! Not all platforms provide an efficient `Condvar` implementation: the UNIX | ||
| //! `pthread_condvar_t` needs memory allocation, while SGX doesn't have | ||
| //! synchronization primitives at all. Therefore, we implement our own. |
There was a problem hiding this comment.
Do we think this is the right tradeoff? It looks like the affected platforms are fairly "rare", and I'm not sure it makes sense for std to be maintaining relatively complex data structures for such platforms, especially if they're not even getting lightly tested in our CI.
There was a problem hiding this comment.
The problem is that every couple of months someone comes along and adds a completely new platform to std. Understandably, not every target maintainer is well-versed in writing synchronization primitives, leading to bugs like #114581 on SGX. Actually, SGX is an interesting case, because the environment only provides thread parking, so there'll always be the need for some queue like this one. The idea behind this PR is that this implementation will be used as the default on every new target, unless they have a good reason not to, so that we only have to maintain one admittedly complex implementation instead of five.
There are simpler implementations that I could switch to, if desired (the NetBSD pthread_condvar for example uses a stack of waiters and spins on contended access to it). It's just that making the shared implementation really good seemed like a good idea to me (it also might make it easier to convince people that they really shouldn't bring their own).
There was a problem hiding this comment.
I see. Yeah, if this is intended to be the fallback queue absent platform-specific, built-in queues that make more sense, then that makes more sense to me.
| //! only a pointer in size. | ||
| //! | ||
| //! This implementation is loosely based upon the lockless `Condvar` in | ||
| //! [`usync`](https://github.com/kprotty/usync/blob/8937bb77963f6bf9068e56ad46133e933eb79974/src/condvar.rs). |
There was a problem hiding this comment.
This worries me, since that repository is only MIT licensed -- and std needs to also be Apache licensed.
There was a problem hiding this comment.
This implementation shares no code with usync and contains significant alterations to the algorithm, so I don't think that this is a problem. Maybe "inspired by" is a better way to put it.
CC @kprotty what do you think?
| } | ||
| } | ||
|
|
||
| #[repr(align(16))] |
There was a problem hiding this comment.
Can we add a comment here on why this is 16, and ideally also an assert somewhere that ensures our alignment is sufficient that MASK bits are never set?
|
|
||
| impl Node { | ||
| unsafe fn notify(node: NonNull<Node>) { | ||
| let thread = unsafe { node.as_ref().thread.clone() }; |
There was a problem hiding this comment.
Why do we need to clone the thread here?
Also, can we add safety requirements + justifications on the unsafe functions and blocks added by this PR?
|
@he32 It would be nice if you could run the rustc test suite on |
There was a problem hiding this comment.
I'm worried about the testing for this given the limited platform coverage and relatively tricky code. Can we at least run some rudimentary, single-thread tests against it on linux? Reviewing the code manually it looks reasonably OK to me (some additional comments in this PR).
| // mutably accessed or destroyed while other threads may | ||
| // be accessing it. Guard against unwinds using a panic | ||
| // guard that aborts when dropped. | ||
| let guard = PanicGuard; |
There was a problem hiding this comment.
It seems like this should get moved up to before we register the node, no?
| /// # Safety | ||
| /// May only be called from the thread to which this handle belongs. | ||
| pub(crate) unsafe fn park_timeout(&self, dur: Duration) { | ||
| unsafe { self.inner.as_ref().parker().park_timeout(dur) } |
There was a problem hiding this comment.
The public park_timeout has a PanicGuard -- should this also have that?
It looks like the ~only advantage to this is that we avoid re-fetching the handle from thread local storage, is that just a performance optimization?
joboet
added
S-waiting-on-author
alex-semenyuk
added
S-waiting-on-author
Like #110211, this PR adds a new
Condvarimplementation based on a lockless queue ofThreads. Unfortunately, we cannot entirely eliminate the UNIX condvar code because of the thread priority properties it provides on platforms like macOS. But for NetBSD, Windows 7, SGX, Xous and hopefully some new targets these properties are not relevant.Tested on macOS (modify the
cfg_ifinsys/sync/condvar/mod.rsto test locally). As far as I am aware, none of the relevant platforms are tested in CI unfortunately.