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

Add a document describing a safe API for Tock 2.0's Subscribe syscall. #340

Closed
wants to merge 3 commits into from

Conversation

jrvanwhy
Copy link
Collaborator

Creating a safe, sound API for Subscribe that supports callbacks allocated on the stack is nontrivial, because it is difficult to guarantee the subscription is cleaned up before the callback is deallocated. This design, which is a continuation of #338, is an attempt to create a safe API for Subscribe.

It is inspired by core::pin, but for efficiency it uses a distinct handle type (SubscribeHandle) that is zero-sized.

I intend to have this PR reviewed by reviewers who are unfamiliar with Tock, and I wrote this document accordingly.

Rendered

Creating a safe, sound API for Subscribe that supports callbacks allocated on
the stack is nontrivial, because it is difficult to guarantee the subscription
is cleaned up before the callback is deallocated. This design, which is a
continuation of tock#338, is an attempt to
create a safe API for Subscribe.

It is inspired by `core::pin`, but for efficiency it uses a distinct handle type
(`SubscribeHandle`) that is zero-sized.

I intend to have this PR reviewed by reviewers who are unfamiliar with Tock, and
I wrote this document accordingly.

[Rendered](https://github.com/jrvanwhy/libtock-rs/blob/subscribe-api-design/doc/SubscribeApiDesign.md)
@jrvanwhy jrvanwhy added the significant Indicates a PR is significant as defined by the code review policy. label Oct 28, 2021
Copy link

@coryayers24 coryayers24 left a comment

Choose a reason for hiding this comment

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

Test

The advantage of having the `ID` parameter is it allows for a single type (e.g. a driver) to accept multiple callbacks. However, as was pointed out in the review comics, this causes `Callback::callback` (as well as the `extern "C"` shim that calls it) to be monomorphized for each subscribe ID, which adds code size.

Ti50 prefers a design where there are very few distinct Subscribe callbacks, each of which writes a value into a `Cell<Option<[u32; 3]>>` (or similar) on the stack. That design may result in a smaller code size, but increases RAM usage.

Removing `ID` removes the ability to accept multiple callbacks, but it also allows for Ti50 to achieve the tradeoffs they want to achieve. It's not clear to me whether there will be a single type that wants to listen to multiple callbacks, but if we need to we can add a disambiguator in the following manner without causing excessive monomorphization:

```rust
struct DefaultCallback;

// The Disambiguator type should be DefaultCallback *unless* this type
// implements multiple callbacks, in which case each callback would have
// a different Disambiguator type.
trait Callback<Disambiguator = DefaultCallback> {
    fn callback(&self, args: [u32; 3]);
}
```
doc/SubscribeApiDesign.md Outdated Show resolved Hide resolved
doc/SubscribeApiDesign.md Outdated Show resolved Hide resolved
doc/SubscribeApiDesign.md Show resolved Hide resolved
@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Nov 3, 2021

Note to the other libtock-rs maintainers: I will merge this PR myself -- I still have another round of reviews planned.

@ComputerDruid
Copy link

I think that the subscribe_handle! macro can be fooled with an async block like:

let fut = async {
    let app = App;
    subscribe_handle!(handle);
    app.run(handle);
    // some kind of yield point
    futures::future::pending().await
}
// poll once to actually do the subscribe, handle and unsubscriber are still captured in the future
fut.poll(some_context_whatever())
// forgetting the future means we never drop the captured stuff.
std::mem::forget(fut)

Code is an approximation but if it would help I believe I could write a version that compiled.

@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Nov 3, 2021

// poll once to actually do the subscribe, handle and unsubscriber are still captured in the future
fut.poll(some_context_whatever())
// forgetting the future means we never drop the captured stuff.
std::mem::forget(fut)

Future::poll takes self as a Pin<&mut Self>, so it is subject to the following drop guarantee:

Concretely, for pinned data you have to maintain the invariant that its memory will not get invalidated or repurposed from the moment it gets pinned until when drop is called.

Unless I'm misreading something, I believe that you cannot forget a Future like that. Am I misreading?

@ComputerDruid
Copy link

Good point about pinning, but I think you still have a problem if you put it in a box and leak that, which doesn't invalidate those requirements.

@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Nov 3, 2021

Good point about pinning, but I think you still have a problem if you put it in a box and leak that, which doesn't invalidate those requirements.

I don't quite see what you mean -- would you mind providing example code? See this comment for a playground link that can get you started (so you don't have to copy + paste my API manually into the playground or a crate).

@ComputerDruid
Copy link

Thanks for the playground link, that was very helpful!

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3189cb479bbc51d0497ee15947ee458e

Is what I came up with. I also added a couple of println!()s which I think show that subscribe is called but not unsubscribe.

@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Nov 3, 2021

Thanks for the playground link, that was very helpful!

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3189cb479bbc51d0497ee15947ee458e

Is what I came up with. I also added a couple of println!()s which I think show that subscribe is called but not unsubscribe.

I agree that subscribe is called without an unsubscribe -- but I don't think the unsubscribe needs to be called, because the callback is leaked as well. In other words, looking at the safety requirement on kernel_subscribe:

// Safety requirement: The process must do one of the following before the
// `callback` argument becomes invalid:
// 1. Overwrite this callback using another `kernel_subscribe` call with the
//    same ID.
// 2. Erase this callback using a `kernel_unsubscribe` call with the same ID.

This is trivially satisfied because the callback argument remains valid indefinitely. This will certainly produce some weird behavior (the callback potentially continuing to execute after the future has been forgotten), but I think it's still sound.

@ComputerDruid
Copy link

the callback argument remains valid indefinitely

Yes, that's true. But moving the App outside the future still compiles:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5ad3cbf904097a004e565b3d38290553

Which I believe violates those rules.

@alexandruradovici
Copy link
Contributor

I might be wrong, but from your example it seems that a driver (represented here by a a data type) is able to define only one single callback as it is limited to one single impl of the Callback trait. What about adding a generic type to the callback trait:

trait Callback<const ID: usize> {
    fn invoke(&self, arg: u32);
}

This would allow multiple implementations of the trait, ID being anything the driver wants, maybe it could be mapped to the subscribe number.

@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Nov 5, 2021

I might be wrong, but from your example it seems that a driver (represented here by a a data type) is able to define only one single callback as it is limited to one single impl of the Callback trait. What about adding a generic type to the callback trait:

trait Callback<const ID: usize> {
    fn invoke(&self, arg: u32);
}

This would allow multiple implementations of the trait, ID being anything the driver wants, maybe it could be mapped to the subscribe number.

That concern was already discussed here: #340 (comment).

The problem with adding a const ID: u32 generic argument is that some codebases will use the same callback for many drivers, such as a callback that just sets a done: Cell<bool> flag to true.

A solution I am more likely to pick is something like the following:

trait AllowsId<const ID: u32> {}

// SupportedIds is a type that implements one or more AllowsIds traits. This callback
// can only be called with a subscription ID that SupportedIds includes.
trait Callback<SupportedIds> {
    fn invoke(&self, arg: u32);
}

// Supported ID list for callbacks that work with any subscribe ID.
struct AnyId;
impl<const ID: u32> AllowsId<ID> for AnyId {}

// Supported ID list for callbacks that support exactly one subscribe ID.
struct OneId<const ID: u32>;
impl<const ID: u32> AllowsId<ID> for OneId<ID> {}

Then you can have a mix of callback types, without unnecessary monomorphization bloat.

Regardless, I'm not going to add all those details to my high-level design discussions such as this document and #341 , because it's not relevant to the safety of the API.

@alexandruradovici
Copy link
Contributor

Sorry, I didn't see the comment. Thanks, makes a lot of sense now.

@kupiakos
Copy link
Contributor

kupiakos commented Nov 8, 2021

I think the best route here is to have the callback data owned by the same struct that has the Pin drop guarantee. That way Pin can do its job of ensuring destructors run before deallocation. Something like this playground? Is there any way to break this with async wizardry @ComputerDruid (or should I call it async druidcraft?)

It's less flexible due to needing moves, but very simple - I think that's a good thing. I added an unsubscribe as well to be able to reuse the same callback object for multiple subscriptions.

@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Nov 9, 2021

Closing this PR as it is unsound. I'm beginning to implement the scope guard design from #341, but I'll keep the design from #340 (comment) as a backup in case #341 doesn't work.

@jrvanwhy jrvanwhy closed this Nov 9, 2021
@ComputerDruid
Copy link

ComputerDruid commented Nov 10, 2021

@kupiakos Sorry, I don't think the approach from #340 (comment) works. Moving the callback into the SubscribeHandle isn't enough to make it safe to call the callback; it might still contain references to borrowed data which might no longer be valid.

Example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3c83d65f34bffaacfb72d2089477dbc6

@jrvanwhy
Copy link
Collaborator Author

@kupiakos Sorry, I don't think the approach from #340 (comment) works. Moving the callback into the SubscribeHandle isn't enough to make it safe to call the callback; it might still contain references to borrowed data which might no longer be valid.

Example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3c83d65f34bffaacfb72d2089477dbc6

Thank you for pointing that out! Hopefully the scope guard design is sound; fortunately I am more confident in that design than I was in this design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
significant Indicates a PR is significant as defined by the code review policy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants