Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IPC Server based on future executor #384

Merged
merged 26 commits into from
Aug 16, 2019
Merged

Conversation

roblabla
Copy link
Member

After hitting so many lifetime errors, deadlocks, and weird compiler bugs, we're finally there: A futures implementation that works!

Note that this is based on a snapshot of @Orycterope's TLS work, which is not ready for prime-time (yet). It will get rebased on master once TLS is merged in.

There are still a few things to be done: We're missing a metric shitton of documentation, the future executor should be split from the IPC implementation, etc...

@roblabla roblabla marked this pull request as ready for review July 31, 2019 16:21
@todo
Copy link

todo bot commented Jul 31, 2019

How to know which handle was signaled ?_?.

// TODO: How to know which handle was signaled ?_?.
pub(crate) fn wait_for(&self, handles: &[HandleRef], ctx: &mut Context) {
for handle in handles {
self.0.lock().push_back(WorkItem::WaitHandle(handle.staticify(), ctx.waker().clone()))
}
}
/// Spawn a top-level future on the event loop. The future will be polled once
/// on spawn. Once the future is spawned, it will be owned by the [WaitableManager].
pub fn spawn(&self, future: FutureObj<'a, ()>) {
self.0.lock().push_back(WorkItem::Spawn(future));


This comment was generated by todo based on a TODO comment in 10bf5f6 in #384. cc @roblabla.

@todo
Copy link

todo bot commented Jul 31, 2019

Handle other types.

// TODO: Handle other types.
Some((4, cmdid)) | Some((6, cmdid)) => dispatch.call((&mut object, work_queue.clone(), cmdid, &mut buf[..])).await
.map(|_| false)
.unwrap_or_else(|err| { error!("Dispatch method errored out: {:?}", err); true }),
Some((2, _)) => true,
_ => true
};
if close {
break;
}


This comment was generated by todo based on a TODO comment in 10bf5f6 in #384. cc @roblabla.

@todo
Copy link

todo bot commented Jul 31, 2019

Explain what [crate::ipc::WorkQueue] is

// TODO: Explain what [crate::ipc::WorkQueue] is
// TODO: Somehow make polling multiple handle possible.
fn wait_async<'b>(&self, queue: crate::futures::WorkQueue<'b>)-> impl core::future::Future<Output = ()> + Unpin +'b {
struct MyFuture<'a> {
is_done: bool, queue: crate::futures::WorkQueue<'a>, handle: HandleRef<'static>
}
impl<'a> core::future::Future for MyFuture<'a> {
type Output = ();
fn poll(mut self: core::pin::Pin<&mut Self>, cx: &mut core::task::Context) -> core::task::Poll<()> {
if self.is_done {
core::task::Poll::Ready(())


This comment was generated by todo based on a TODO comment in 10bf5f6 in #384. cc @roblabla.

@todo
Copy link

todo bot commented Jul 31, 2019

Somehow make polling multiple handle possible.

// TODO: Somehow make polling multiple handle possible.
fn wait_async<'b>(&self, queue: crate::futures::WorkQueue<'b>)-> impl core::future::Future<Output = ()> + Unpin +'b {
struct MyFuture<'a> {
is_done: bool, queue: crate::futures::WorkQueue<'a>, handle: HandleRef<'static>
}
impl<'a> core::future::Future for MyFuture<'a> {
type Output = ();
fn poll(mut self: core::pin::Pin<&mut Self>, cx: &mut core::task::Context) -> core::task::Poll<()> {
if self.is_done {
core::task::Poll::Ready(())
} else {


This comment was generated by todo based on a TODO comment in 10bf5f6 in #384. cc @roblabla.

@todo
Copy link

todo bot commented Jul 31, 2019

Implement a futures-based condvar instead of using event for in-process eventing.

A futures-based condvar can easily be implemented entirely in userspace, without the need for any kernel help. It would have a lot less overhead than using a kernel Event.


SunriseOS/sm/src/main.rs

Lines 72 to 81 in 10bf5f6

// TODO: Implement a futures-based condvar instead of using event for in-process eventing.
// BODY: A futures-based condvar can easily be implemented entirely in userspace, without
// BODY: the need for any kernel help. It would have a lot less overhead than using a kernel Event.
static ref SERVICES_EVENT: (WritableEvent, ReadableEvent) = {
crate::libuser::syscalls::create_event().unwrap()
};
}
/// Get the length of a service encoded as an u64.
#[allow(clippy::verbose_bit_mask)] // More readable this way...


This comment was generated by todo based on a TODO comment in 10bf5f6 in #384. cc @roblabla.

@todo
Copy link

todo bot commented Jul 31, 2019

Find a way to clean up the shared service handles.

Service handles returned by `new()` are valid throughout the lifetime of the process. Currently, they get cleaned up automatically on process exit. This is not ideal: resources should get automatically cleaned up through the appropriate calls to CloseHandle (especially for "real" homebrew, which don't automatically release leaked resources).


// TODO: Find a way to clean up the shared service handles.
// BODY: Service handles returned by `new()` are valid throughout the lifetime
// BODY: of the process. Currently, they get cleaned up automatically on process
// BODY: exit. This is not ideal: resources should get automatically cleaned up
// BODY: through the appropriate calls to CloseHandle (especially for "real"
// BODY: homebrew, which don't automatically release leaked resources).
writeln!(s, " /// Acquires the shared handle to the `{}` service - connecting if it wasn't already.", service).unwrap();
writeln!(s, " pub fn new{}() -> Result<&'static {}, Error> {{", name, struct_name).unwrap();
writeln!(s, " static HANDLE : spin::Once<{}> = spin::Once::new();", struct_name).unwrap();
writeln!(s, " if let Some(s) = HANDLE.r#try() {{").unwrap();
writeln!(s, " Ok(s)").unwrap();


This comment was generated by todo based on a TODO comment in 10bf5f6 in #384. cc @roblabla.

Copy link
Member

@marysaka marysaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything is okay apat from this duplicate change

flate2 = { version = "1.0", features = ["rust_backend"] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already in PR #398

Copy link
Member

@Orycterope Orycterope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes.await

ipcdefs/example.id Outdated Show resolved Hide resolved
kernel/src/event.rs Show resolved Hide resolved
kernel/src/interrupts/syscalls.rs Outdated Show resolved Hide resolved
kernel/src/interrupts/syscalls.rs Outdated Show resolved Hide resolved
libuser/src/futures.rs Outdated Show resolved Hide resolved
sm/src/main.rs Outdated Show resolved Hide resolved
sm/src/main.rs Outdated Show resolved Hide resolved
sm/src/main.rs Outdated Show resolved Hide resolved
sm/src/main.rs Outdated Show resolved Hide resolved
time/src/timezone.rs Show resolved Hide resolved
@todo
Copy link

todo bot commented Aug 12, 2019

Move sm::ServiceName inside sunrise_libuser::sm.

Sm has a ServiceName transparent struct that allows easy displaying of u64-encoded service names. Ideally, ServiceName should be generated by swipc-gen, and libuser would inject a Display implementation in it.


SunriseOS/sm/src/main.rs

Lines 107 to 117 in 306d95b

// TODO: Move sm::ServiceName inside sunrise_libuser::sm.
// BODY: Sm has a ServiceName transparent struct that allows easy displaying of
// BODY: u64-encoded service names. Ideally, ServiceName should be generated by
// BODY: swipc-gen, and libuser would inject a Display implementation in it.
#[repr(transparent)]
#[derive(Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)]
struct ServiceName(u64);
impl core::fmt::Debug for ServiceName {
fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
if let Ok(s) = get_service_str(&self.0) {


This comment was generated by todo based on a TODO comment in 306d95b in #384. cc @roblabla.

@todo
Copy link

todo bot commented Aug 14, 2019

Handle timeouts in wait_sync

// TODO: Handle timeouts in wait_sync
},
Err(KernelError::InvalidHandle) /* | Err(KernelError::Interrupted) */ => {
// We'll need to wake up every future, and let the culprit
// deal with the mess. There isn't a better way
// unfortunately, as the kernel does not tell us which
// handle is invalid.
for hnd_wakers in handle_to_waker.drain(..) {
for (_, item) in hnd_wakers {
item.wake()
}


This comment was generated by todo based on a TODO comment in e5a7cda in #384. cc @roblabla.

kernel/src/syscalls.rs Show resolved Hide resolved
@todo
Copy link

todo bot commented Aug 16, 2019

Footgun: Sharing ServerPorts can result in blocking the event loop

If the user shares ServerPorts across threads/futures and does something like calling accept straight after wait_async, they might end up blocking the event loop. This is because two threads might race for the call to accept after the wait_async.


// TODO: Footgun: Sharing ServerPorts can result in blocking the event loop
// BODY: If the user shares ServerPorts across threads/futures and does
// BODY: something like calling accept straight after wait_async, they might
// BODY: end up blocking the event loop. This is because two threads might
// BODY: race for the call to accept after the wait_async.
pub fn wait_async<'a>(&self, queue: crate::futures::WorkQueue<'a>) -> impl core::future::Future<Output = Result<(), Error>> + Unpin + 'a {
self.0.as_ref().wait_async(queue)
}
}
/// A Thread. Created with the [create_thread syscall].


This comment was generated by todo based on a TODO comment in da88164 in #384. cc @roblabla.

There's no reason to force proxy functions to use a &mut self, since
sendSyncRequest only takes a &self. Multiple IPC requests will get
queued up by the kernel, and sent one after the other.
Introduce a Proxy::new() function, which caches the returned handle to
avoid creating new handles each time we need to access a service. Those
handle will be kept in a static Once, and never get dropped.

NOTE: We will want to eventually find a way to clean up the handles on
process exit.
WaitSynchronization should not reschedule when called with a timeout of
0. This allows using waitsync as a way to check if a handle is signaled
in a non-blocking way, which will become very important. Additionally,
it allows using ReplyAndReceive in a non-blocking way.
Spurious interrupts are now prevented by having the wait_async function
do a non-blocking check that the handle is, indeed, signaled before
returning Ready. This should allow using Futures with select, along with
making us more robust against being woken up erroneously.

We separately make sure to clean up wait handles on task completion, to
avoid unbounded memory growth.
We now unregisters the current task from waiting when dropping the
future returned by wait_async. This should avoid various spurious
wakeups conditions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants