-
Notifications
You must be signed in to change notification settings - Fork 109
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
Conversation
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)
There was a problem hiding this 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]); } ```
…used to invoke callbacks.
Note to the other |
I think that the 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. |
Future::poll takes
Unless I'm misreading something, I believe that you cannot forget a |
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). |
Thanks for the playground link, that was very helpful! Is what I came up with. I also added a couple of |
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 // 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 |
Yes, that's true. But moving the App outside the future still compiles: Which I believe violates those rules. |
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 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 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. |
Sorry, I didn't see the comment. Thanks, makes a lot of sense now. |
I think the best route here is to have the callback data owned by the same struct that has the It's less flexible due to needing moves, but very simple - I think that's a good thing. I added an |
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. |
@kupiakos Sorry, I don't think the approach from #340 (comment) works. Moving the callback into the |
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. |
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