Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upUse the parking_lot locking primitives #56410
Conversation
rust-highfive
assigned
withoutboats
Dec 1, 2018
This comment was marked as outdated.
This comment was marked as outdated.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
label
Dec 1, 2018
This comment has been minimized.
This comment has been minimized.
|
r? @RalfJung |
rust-highfive
assigned
RalfJung
and unassigned
withoutboats
Dec 1, 2018
faern
force-pushed the
faern:add-parking-lot
branch
from
93093dc
to
57526ff
Dec 1, 2018
This comment has been minimized.
This comment has been minimized.
|
Just to be clear, this PR only changes the "back-end" of the mutex implementation. There are no changes to the public |
This comment has been minimized.
This comment has been minimized.
Isn’t there a difference between fairness of native EDIT: on some of the platforms, that is. |
This comment has been minimized.
This comment has been minimized.
Any fairness was "implementation-defined" in the current implementation. |
Centril
reviewed
Dec 1, 2018
|
Some observations; applies generally throughout the PR but I picked a few spots to illustrate. :) |
| pub struct UnparkHandle; | ||
|
|
||
| impl UnparkHandle { | ||
| // Wakes up the parked thread. This should be called after the queue lock is |
This comment has been minimized.
This comment has been minimized.
Centril
Dec 1, 2018
Contributor
Documentation is a bit unclear re. when this is UB and not; do you mean "must" by "should"?
(Do a review of the PR in general to make the safety invariants clearer and more definitive... For example "Behavior is undefined if X")
This comment has been minimized.
This comment has been minimized.
Amanieu
Dec 1, 2018
Contributor
This is an internal API which is only used in the internals of parking_lot_core. Regarding your question: I used "should" here because you don't want to issue a system call while holding a lock, to keep lock durations short.
faern
force-pushed the
faern:add-parking-lot
branch
from
fb5360f
to
b4d3471
Dec 1, 2018
This comment was marked as outdated.
This comment was marked as outdated.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
faern
force-pushed the
faern:add-parking-lot
branch
from
b4d3471
to
e230096
Dec 1, 2018
This comment was marked as outdated.
This comment was marked as outdated.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
FYI, I will not have time to look at this "+4168 −2426" PR this week. I should be able to get to it next week though. I also feel slightly overwhelmed by the importance of this and would appreciate a co-reviewer -- maybe someone who actually has reviewed libstd code before :D |
This comment has been minimized.
This comment has been minimized.
|
I should add that I'm currently working on improving this code. Will likely push changes in a day or two. I have already backported some of the feedback here directly to |
faern
force-pushed the
faern:add-parking-lot
branch
from
c62d50d
to
910670f
Dec 4, 2018
This comment has been minimized.
This comment has been minimized.
|
|
faern
force-pushed the
faern:add-parking-lot
branch
3 times, most recently
from
abc062b
to
9718fba
Dec 9, 2018
This comment has been minimized.
This comment has been minimized.
|
I could post some kind of benchmarks. The MutexBuilt using latest nightly rustc/libstd:
Built using the code currently in this PR (stage1 compiler):
RwLockBuilt using latest nightly rustc/libstd:
Built using the code currently in this PR (stage1 compiler):
All of the above was on Linux. |
faern
force-pushed the
faern:add-parking-lot
branch
from
9718fba
to
1407c89
Dec 9, 2018
This comment was marked as outdated.
This comment was marked as outdated.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
I was able to remove the boxing of the inner locking type in the EDIT: Worth noting that this also affects the size of the synchronization structs. |
faern
added some commits
Feb 21, 2019
faern
force-pushed the
faern:add-parking-lot
branch
from
b56c3fa
to
4078d36
Apr 3, 2019
faern
force-pushed the
faern:add-parking-lot
branch
from
4078d36
to
e0d5bc1
Apr 3, 2019
This comment has been minimized.
This comment has been minimized.
|
@bors try |
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Apr 3, 2019
jethrogb
reviewed
Apr 3, 2019
| /// The value of a newly initialized `RwLock`. Which happens to be | ||
| /// `RawRwLock::INIT` (a zeroed `usize`), a false boolean (zero) | ||
| /// and then padding. | ||
| const RWLOCK_INIT: &[u8] = &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]; |
This comment has been minimized.
This comment has been minimized.
jethrogb
Apr 3, 2019
Contributor
As mentioned in the comment just above, src/UnwindRustSgx.h in libunwind needs to be updated, and this PR will need to pull in the changes in src/ci/docker/dist-various-2/Dockerfile. @VardhanThigle can you help with this?
This comment has been minimized.
This comment has been minimized.
faern
Apr 3, 2019
•
Author
Contributor
I'm aware, but good you pointed it out. This won't be merged just yet, we just do a test run. But we can probably go ahead and create a PR in libunwind for the change. Here you go: fortanix/libunwind#4
It's a bit of a chicken and egg problem. But the libunwind PR must be merged first, so we can update the commit id in the dockerfile in this PR. I assume that's the way to go.
Would be awesome if you two who are familiar with the platform could verify that these two PRs work together as intended.
This comment has been minimized.
This comment has been minimized.
faern
Apr 3, 2019
Author
Contributor
I see that libunwind is not a submodule of your llvm-project repository. The files seem copied over? I think it's best if you update your libraries as you see fit and then ping me with the new commit id :)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
faern
Apr 4, 2019
Author
Contributor
The link https://github.com/fortanix/llvm-project/archive/cc75f6e51b7dafe1dbd2ad16d969fcd3ff446053.tar.gz does not work. Nor do I see any new commits pushed to llvm-project
This comment has been minimized.
This comment has been minimized.
jethrogb
Apr 4, 2019
•
Contributor
I'm seeing a hang in std::io::buffered::tests::panic_in_write_doesnt_flush_in_drop after the second thread has exited, join never returns. Backtrace excerpt:
#0 0x00007ffff00b33a6 in std::parking_lot::condvar::Condvar::wait_until_internal::haec9e821d58c63ec ()
#1 0x00007ffff01192ae in std::thread::park::h55b748bc253e86f5 ()
#2 0x00007ffff007cbb2 in std::sync::mpsc::oneshot::Packet$LT$T$GT$::recv::h9344b3a4c5e28c0d ()
#3 0x00007ffff000dfce in std::sync::mpsc::Receiver$LT$T$GT$::recv::h18960999ebdd9329 ()
#4 0x00007ffff0119c9e in std::thread::JoinHandle$LT$T$GT$::join::h5220ab83fba0a8b1 ()
#5 0x00007ffff00955ee in std::io::buffered::tests::panic_in_write_doesnt_flush_in_drop::hb734fd606fd827e8 ()
(It's stuck in the wait usercall).
I'm still trying to figure out if this is a bug in unwinding or in parking_lot's SGX implementation.
This comment has been minimized.
This comment has been minimized.
faern
Apr 4, 2019
Author
Contributor
This backtrace, and your description, sounds very similar to the hangs we saw when discovering the problem around fork() and parking_lot not being fork safe. See these threads: #56410 (comment) and #56410 (comment)
I have no idea if this is remotely related. But just dropping this info here for now.
This comment has been minimized.
This comment has been minimized.
faern
Apr 4, 2019
Author
Contributor
Your backtrace shows the still alive thread and that it's waiting for the spawned thread to return its exiting result. I don't think the bug is in any code running in this thread. I think the interesting part is how the other thread exits and what stops it from storing/sending its exit value before dying. Could as you say very well be a bug in the unwinding code for example.
This comment has been minimized.
This comment has been minimized.
faern
Apr 4, 2019
•
Author
Contributor
I don't really get how it's supposed to work. I'm looking at this code: https://github.com/rust-lang/rust/blob/master/src/libstd/sys/sgx/thread.rs#L32-L33
Is this where it executes the closure that is the thread's closure? It just runs the closure, without catching any panics. So if it unwinds, will it ever reach let _ = self.done.send(()); and actually tell the parent thread that it's done?
EDIT: Err.. The panic catching is done much higher up. Never mind me. I need to stop looking at code for today.
This comment has been minimized.
This comment has been minimized.
jethrogb
Apr 4, 2019
•
Contributor
Ok, the second thread does call notify_one_slow, but it never calls send. Instead, it just calls insecure_time (via Instant::now) a lot (this is extremely slow on SGX), and then exits.
faern
referenced this pull request
Apr 3, 2019
Merged
Update RWLOCK_INIT with new parking_lot based value #4
This comment has been minimized.
This comment has been minimized.
|
|
faern commentedDec 1, 2018
•
edited by RalfJung
This PR adds the
parking_lotcode to libstd and uses it for thesys_common::{mutex,rwlock,condvar,remutex}implementations.This has been discussed in https://internals.rust-lang.org/t/standard-library-synchronization-primitives-and-undefined-behavior/8439/9
Thanks @Amanieu for mentoring when doing this, and basically all the code is his as well of course.
Fixes #35836
Fixes #53127