Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
mzohreva committed Jul 11, 2020
1 parent c457b67 commit 1466598
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 39 deletions.
31 changes: 15 additions & 16 deletions src/libstd/sys/sgx/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,9 @@ pub fn decode_error_kind(code: i32) -> ErrorKind {
// of time and timeouts in SGX model. The enclave runner serving usercalls may
// lie about current time and/or ignore timeout values.
//
// Once the event is observed, `woken_up` will be used to determine whether or
// not the event was spurious.
//
// FIXME: note these caveats in documentation of all public types that use this
// function in their execution path.
pub fn wait_timeout_sgx<F>(event_mask: u64, duration: crate::time::Duration, woken_up: F)
// Once the event is observed, `should_wake_up` will be used to determine
// whether or not the event was spurious.
pub fn usercall_wait_timeout<F>(event_mask: u64, duration: crate::time::Duration, should_wake_up: F)
where
F: Fn() -> bool,
{
Expand All @@ -141,7 +138,9 @@ where
if event_mask == 0 {
rtabort!("expected usercalls::wait() to return Err, found Ok.");
}
rtassert!(eventset & event_mask == event_mask);
// A matching event is one whose bits are equal to or a subset
// of `event_mask`.
rtassert!(eventset & !event_mask == 0);
true
}
Err(e) => {
Expand All @@ -152,18 +151,18 @@ where
}

match wait_checked(event_mask, Some(duration)) {
false => return, // timed out
true if woken_up() => return, // woken up
true => {} // spurious event
false => return, // timed out
true if should_wake_up() => return, // woken up
true => {} // spurious event
}

// Drain all cached events.
// Note that `event_mask != 0` is implied if we get here.
loop {
match wait_checked(event_mask, None) {
false => break, // no more cached events
true if woken_up() => return, // woken up
true => {} // spurious event
false => break, // no more cached events
true if should_wake_up() => return, // woken up
true => {} // spurious event
}
}

Expand All @@ -176,9 +175,9 @@ where
let mut remaining = duration;
loop {
match wait_checked(event_mask, Some(remaining)) {
false => return, // timed out
true if woken_up() => return, // woken up
true => {} // spurious event
false => return, // timed out
true if should_wake_up() => return, // woken up
true => {} // spurious event
}
remaining = match duration.checked_sub(start.elapsed()) {
Some(remaining) => remaining,
Expand Down
5 changes: 2 additions & 3 deletions src/libstd/sys/sgx/thread.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
#![cfg_attr(test, allow(dead_code))] // why is this necessary?

use crate::ffi::CStr;
use crate::io;
use crate::sys::wait_timeout_sgx;
use crate::sys::usercall_wait_timeout;
use crate::time::Duration;

use super::abi::usercalls;
Expand Down Expand Up @@ -76,7 +75,7 @@ impl Thread {
}

pub fn sleep(dur: Duration) {
wait_timeout_sgx(0, dur, || true);
usercall_wait_timeout(0, dur, || true);
}

pub fn join(self) {
Expand Down
37 changes: 17 additions & 20 deletions src/libstd/sys/sgx/waitqueue.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
/// A simple queue implementation for synchronization primitives.
///
/// This queue is used to implement condition variable and mutexes.
///
/// Users of this API are expected to use the `WaitVariable<T>` type. Since
/// that type is not `Sync`, it needs to be protected by e.g., a `SpinMutex` to
/// allow shared access.
///
/// Since userspace may send spurious wake-ups, the wakeup event state is
/// recorded in the enclave. The wakeup event state is protected by a spinlock.
/// The queue and associated wait state are stored in a `WaitVariable`.
//! A simple queue implementation for synchronization primitives.
//!
//! This queue is used to implement condition variable and mutexes.
//!
//! Users of this API are expected to use the `WaitVariable<T>` type. Since
//! that type is not `Sync`, it needs to be protected by e.g., a `SpinMutex` to
//! allow shared access.
//!
//! Since userspace may send spurious wake-ups, the wakeup event state is
//! recorded in the enclave. The wakeup event state is protected by a spinlock.
//! The queue and associated wait state are stored in a `WaitVariable`.
use crate::num::NonZeroUsize;
use crate::ops::{Deref, DerefMut};
use crate::sys::wait_timeout_sgx;
use crate::sys::usercall_wait_timeout;
use crate::time::Duration;

use super::abi::thread;
Expand Down Expand Up @@ -176,15 +176,12 @@ impl WaitQueue {
}));
let entry_lock = lock.lock().queue.inner.push(&mut entry);
before_wait();
// don't panic, this would invalidate `entry` during unwinding
wait_timeout_sgx(EV_UNPARK, timeout, || entry_lock.lock().wake);
usercall_wait_timeout(EV_UNPARK, timeout, || entry_lock.lock().wake);
// acquire the wait queue's lock first to avoid deadlock.
let mut guard = lock.lock();
let entry_guard = entry_lock.lock();
let success = entry_guard.wake;
let success = entry_lock.lock().wake;
if !success {
// nobody is waking us up, so remove the entry from the wait queue.
drop(entry_guard);
// nobody is waking us up, so remove our entry from the wait queue.
guard.queue.inner.remove(&mut entry);
}
success
Expand Down Expand Up @@ -363,8 +360,8 @@ mod unsafe_list {
///
/// # Safety
///
/// The caller must ensure that entry has been pushed prior to this
/// call and has not moved since push.
/// The caller must ensure that `entry` has been pushed onto `self`
/// prior to this call and has not moved since then.
pub unsafe fn remove(&mut self, entry: &mut UnsafeListEntry<T>) {
rtassert!(!self.is_empty());
// BEFORE:
Expand Down
1 change: 1 addition & 0 deletions src/test/ui/mpsc_stress.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ impl Barrier {
fn wait(self) {
self.shared.fetch_add(1, Ordering::SeqCst);
while self.shared.load(Ordering::SeqCst) != self.count {
#[cfg(target_env = "sgx")]
thread::yield_now();
}
}
Expand Down

0 comments on commit 1466598

Please sign in to comment.