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

[RFC] generator tasks #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[RFC] generator tasks #22

wants to merge 1 commit into from

Conversation

japaric
Copy link
Contributor

@japaric japaric commented Oct 29, 2019

Summary

Add experimental support (behind a Cargo feature) for generators to Real Time
for The Masses.

Motivation

Support generator sugar (i.e. yield) to ease the creation of interrupt-driven
state machines.

Rendered


Here's an MVP implementation but please note that at the moment it only implements the simplest case of hardware generator tasks -- it doesn't support software tasks or integration with the schedule API.

@japaric japaric requested review from korken89 and a team as code owners October 29, 2019 02:30
@TeXitoi
Copy link

TeXitoi commented Oct 29, 2019

Looks great.

I don't see any advantage to support static variable in the generator constructor. Do you have any?

@japaric
Copy link
Contributor Author

japaric commented Oct 30, 2019

Someone asked on the matrix channel why the example program prints "A" before "idle". Answering here for visibility:

a is a (task generator) constructor, not a (task) handler which is the case of b, a plain (non-generator) task. The function a, like the init function, will run only once; the generator it returns (the value that looks like a closure: || { .. }) is what will be continuously be resumed when task a is triggered (by spawn.a()).

The function a is executed (once) by the runtime after init runs but before idle or any task runs. That's why you get "a" after "init" and before "idle". The generator code (the bit between || { and }) only starts executing when the task is triggered and the task is first triggered in idle so you get "B" after "idle" in the output.

As a rule of thumb you can think of it like this: inside a task generator function, the code outside the "bars" (|| { .. }) is evaluated eagerly (before idle); the code inside the bars is evaluated lazily (first executed when the task is triggered).

@japaric
Copy link
Contributor Author

japaric commented Oct 30, 2019

I don't see any advantage to support static variable in the generator constructor. Do you have any?

Hmm, off the top of my head: having a task local pool allocator is one use case. If you declare the pool inside the task generator function then no other task will be able to use it. Something like this:

#[rtfm::app(/* .. */)]
const APP: () = {
    #[task]
    fn a(cx: a::Context) -> impl Generator<Yield = (), Return = !> {
        static mut M: [u8; 1024] = [0; 1024];

        pool!(A: [u8; 128]); // empty pool

        let m: &'static mut [u8; 1024] = M;
        A::grow(m); // give some memory to the pool

        move || loop {
            // use the pool
            let x = A::alloc();
            // ..
        }
    }

    #[task]
    fn b(cx: b::Context) {
        // pool A cannot be used here
    }
};

One could have declared the pool outside the #[rtfm::app] item and semantics would have been the same so this is only for encapsulation (hiding things). However, if you want to declare the pool inside task a then you need a &'static mut chunk of memory and a static mut variable is the only way to get that in that context. There may be other use cases for &'static mut references in task generators and static mut variables is the only way to get that kind of reference.

@japaric
Copy link
Contributor Author

japaric commented Nov 5, 2019

I want to address @perlindgren 's comment from the other thread
(#18 (comment)) about BASEPRI
optimizations and resume arguments here.

I see two ways to recover the BASEPRI optimization but one of them may actually
be unsound (?). Let me start with the seemingly sound one.

resume(priority)

The idea is to pass the Priority value, which tracks the dynamic priority,
back into the generator on each resumption. The lock API would have to be
modified to explicitly take this Priority value. An user application would
look like this:

#[task(resources = [x])]
fn foo(cx: foo::Context) -> impl Generator<Yield = (), Return = !> {
    move |priority: &Priority| loop {
        cx.resources.x.lock(priority, |x: &mut i64| {
            //              ^^^^^^^^
            *x += 1;
        });

        // suspend until next resumption
        let priority: &Priority = yield;
        cx.resources.x.lock(priority, |x| {
             // ..
        });
    }
}

The code produced by the framework would look like this:

type G = impl Generator<Yield = (), Return = !>;
static mut X: MaybeUninit<G> = MaybeUninit::uninit();

#[no_mangle]
fn UART0() {
    const PRIORITY: u8 = 1; // static priority
    let priority = Priority(Cell::new(PRIORITY)); // dynamic priority
    Pin::new_unchecked(&mut *G.as_mut_ptr()).resume(priority);
    //                                              ^^^^^^^^
}

This requires more language support: it requires not only resume with
arguments but it also needs support for resumption arguments that have
lifetimes. No semantics have been specified for resumption arguments but I
expect that references (or any non-: 'static value) passed into a generator
will be "killed" between suspension points or at the very least it should not
be possible to store these values in the generator state. This is required for
safety; consider the following example:

let mut g = |a: &i32|  {
     println!("{}", a);
     let b = yield;
     println!("{}", a); //~ error: `a` may be invalid at this point
};

pin_mut!(g);

{
    let x = 0;
    g.resume(&x); `a = &x`
}
// `a` has been invalidated at this point
let y = 0;
g.resume(&y); `b = &y`;

Using a in the second print statement is unsound because a (a pointer) may
have been invalidated between resumptions. This is the case in the example: x
is free between the two resume calls.

resume(resource_proxy)

The other idea is to pass the resource proxies (or the whole context) into the
generator on each resume call. In this approach the Context struct would
not contain any resource proxy and the lock method would not need to be
changed. User code would look like this:

#[task(resources = [x])]
fn foo(__: foo::Context) -> impl Generator<Yield = (), Return = !> {
    move |resources: foo::Resources| loop {
        // NOTE No `cx.`
        resources.x.lock(|x: &mut i64| {
            *x += 1;
        });

        let resources: foo::Resources = yield;
        resources.x.lock(priority, |x| {
             // ..
        });
    }
}

The code produced by the framework would look like this:

type G = impl Generator<Yield = (), Return = !>;
static mut X: MaybeUninit<G> = MaybeUninit::uninit();

#[no_mangle]
fn UART0() {
    const PRIORITY: u8 = 1; // static priority
    let priority = Priority(Cell::new(PRIORITY));
    let resources = foo::Resources {
        x: resources::X::new(&priority),
    };
    Pin::new_unchecked(&mut *G.as_mut_ptr()).resume(resources);
    //                                              ^^^^^^^^^
}

Unsoundness can arise if the user preserves a proxy between suspension points.
For example,

#[task(resources = [x])]
fn foo(__: foo::Context) -> impl Generator<Yield = (), Return = !> {
    move |r1: foo::Resources| loop {
        let r2: foo::Resources = yield;

        r1.x.lock(|x: &mut i64| {
            r2.x.lock(|alias: &mut i64| {
                // `x` and `alias` point to the same memory location
            });
        });
    }
}

The code above produces two instances of the resource proxy for X. This lets the
user get two exclusive (&mut-) references to X and break Rust aliasing rule.

foo::Resources<'a> has an implicit, non-'static lifetime so the compiler may
actually reject the above program. It depends on the exact semantics of
resumption arguments but right now no semantics have been specified anywhere.


In conclusion, it is possible to recover the BASEPRI optimization but (a) we
need more generator support in the language and (b) it will require API changes
that make things less ergonomics. I'm personally not sure if the loss in
ergonomics is worth making locks ~2 instructions less expensive.

@perlindgren
Copy link
Contributor

perlindgren commented Nov 5, 2019

Thanks Jorge for the follow up to #18. This was +/- what I expected. Regarding the cost of locks, there is also increased register pressure when reading/storing the old BASEPRI, so in practice the cost may be higher (of not optimizing the lock out/away). In any case, for now it cannot be applied (as we don't have resume arguments), also its not for granted that the compiler would be able to optimize out the locks even with the approach suggested (it would depend on how generators are internally represented in the rust compiler - and later presented to LLVM).

Having resume arguments, can however be useful (as to send in message payloads eg.), so I think still it would be great if the embedded WG can push for that in meetings with the the teams.

@perlindgren perlindgren mentioned this pull request Dec 4, 2019
@MabezDev
Copy link

The implementation of this feature depends on 4 unstable Rust feature behind the
following feature gates: never_type (the ! type), generators (the yield
operator), generator_trait (the core::ops::Generator trait) and
type_alias_impl_trait (type T = impl Trait). As these features are deemed
unstable, they may change, in syntax or semantics, in any new nightly release.
Therefore keeping generator support working on the latest nightly is likely to
significantly increase the maintenance effort.

Given that some time has passed and all but the generator drawbacks has been solved,

Is it worth looking into this again? I would assume the generator and generator_trait are pretty closely coupled such that we can treat them as one unstable feature.

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.

5 participants