Do not box condition variables on Hermit #100583
Conversation
rustbot
added
the
T-libs
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
|
r? @thomcc (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
There was a problem hiding this comment.
I'm not sure I understand the algorithm in use here (I've dug up the paper but it doesn't actually look similar...), but more generally I'd like to see some checking on error codes -- even just debug_assert, although I think the convention we've adopted elsewhere in std is to start using normal assert for system errors.
| sem2 = init_semaphore(&self.sem2); | ||
| } | ||
|
|
||
| (sem1, sem2) | ||
| } | ||
|
|
||
| pub unsafe fn notify_one(&self) { | ||
| if self.counter.load(SeqCst) > 0 { | ||
| self.counter.fetch_sub(1, SeqCst); |
There was a problem hiding this comment.
This isn't part of the code you've changed, but isn't this a race condition?
There was a problem hiding this comment.
Yes it is. There are other issues: if wait_timeout times out, notify_one does not wake up any thread.
| #[cold] | ||
| fn init_semaphore(sem: &AtomicPtr<c_void>) -> *mut c_void { | ||
| let new = unsafe { | ||
| let mut new = MaybeUninit::uninit(); |
There was a problem hiding this comment.
Using MaybeUninit for this is overkill (it's just a pointer) and would result in strange behavior if the sem_init call fails. Given that you ignore it's error, that seems bad.
| fn init_semaphore(sem: &AtomicPtr<c_void>) -> *mut c_void { | ||
| let new = unsafe { | ||
| let mut new = MaybeUninit::uninit(); | ||
| let _ = abi::sem_init(new.as_mut_ptr(), 0); |
There was a problem hiding this comment.
Assuming this returns an error code, we should probably assert its success.
| match sem.compare_exchange(ptr::null_mut(), new, Release, Acquire) { | ||
| Ok(_) => new, | ||
| Err(sem) => unsafe { | ||
| let _ = abi::sem_destroy(new); |
|
Since there are some issues with the condition variable implementation, I have implemented futex support in the Hermit kernel. That allows us to use the well-tested lock implementations Linux uses, making this PR obsolete. |
By lazily initializing the internal semaphores, the condition variables do not need to be wrapped in
LazyBox.Ping @mkroening, @stlankes