Tracking Issue for Mutex Trait RFC [#377]

[korken89]

This is the tracking issue for the Mutex trait RFC #377, the following questions need to be ironed out before the RFC is implemented:

1. • Decide on crate name (core-mutex was proposed, mutex-trait was selected)
2. • Decide on crate location (freestanding crate under the HAL team was proposed)
3. • Add and implement crate (proposed reference implementation: https://github.com/korken89/core-mutex)
4. • Release crate
5. • Create PRs for the ecosystem
     • Identify where the crate needs to be re-exported
6. • Add a book chapter on it?
7. • (optional) Provide reference implementations for different mutexes
8. • (optional) Add to the 1.0 stabilization pipeline

Lets start with 1 and 2.

CC @rust-embedded/all

[Disasm]

As for the name, I suggest mutex-trait or mutex_trait (like stable_deref_trait)

[korken89]

@Disasm Thanks!

---

I think we should have a time period for proposing names and then we go for a vote.
Maybe 2 weeks now for proposing names, and 1 week to vote after that.
What does the rest of @rust-embedded/all think?

[adamgreig]

Having already had a full RFC for the concept I don't think we really need a vote on the name; I'd be happy for you to go ahead with whatever you think is sensible.

[therealprof]

I also don't think we need a vote. It's bikeshedding really...

[almindor]

I agree with @adamgreig and @therealprof. We shouldn't watershed the name choice, it's not that important.

[nickray]

As an outside view: given that

• in the RFC discussion and chat there has been confusion of this being an implementation, and
• this being called the "Mutex Trait RFC"

calling it mutex-trait sounds very reasonable :)

[geomatsi]

Hi @korken89
Link 'this part of the RTFM book' is broken.

[korken89]

Thanks for the input, let's have a small discussion (yay/nay) on mutex-trait and use that. :)

@geomasi, I'll fix it now.

[korken89]

In today's WG meeting we decided to go with mutex-trait, this will not be set in stone until the first release but lets have it as the current name.

[korken89]

Lets continue, @rust-embedded/hal do you agree that this fits under HAL or would you propose another group?

[therealprof]

@korken89 HAL is maybe not ideal, how do we feel about @rust-embedded/resources ?

[jamesmunns]

IMO HAL team (focused on abstractions and code portability) makes more sense than Resources (focused on docs, PR, and guides).

[therealprof]

I don't quite see a Mutex trait as a Hardware abstraction but more of a implementation convention which seems closer to a resource. But hey, just throwing it out there, I'm not too attached to it. ;)

[andre-richter]

You guys are using a different meaning of resource here, where the "resources" that James speaks of is what the WG team refers to ;)

Theres lots of bikeshedding potential here. IMO a mutex is not really abstracting over hardware. You can implement it with intermediary crates that already abstract over e.g. interrupt masking and atomic operations.

What we have here is a classical SW interface, like for example pthreads. First sentence of phtreads manpage:

POSIX.1 specifies a set of interfaces (functions, header files) for threaded programming commonly known as POSIX threads, or Pthreads.

So following the pthreads example, what we are cooking up here is an API.

Edit:
On second thought, the A in API does not make much sense if the trait is used inside a kernel for example, because application doesn't make too much sense there.
Still think we are talking about interface, not abstraction or resource here.

[therealprof]

You guys are using a different meaning of resource here, where the "resources" that James speaks of is what the WG team refers to ;)

I'm certainly aware of the role of the team, especially since I happen to be a member... ;)

Still think we are talking about interface, not abstraction or resource here.

As James said, the role of the team is managing documentation and guidelines and to me the mutex traits feels a lot closer to a guideline for writing embedded software in Rust than hardware abstraction.

[Disasm]

It feels like both HAL and Resources do not match perfectly with the purpose of this mutex crate.
Since

1. we do not want to create another Team for this project
2. @korken89 is already in the Resources team, but not in the HAL team

I suggest placing this crate inside the Resources team, just to save time required for this to happen.

[almindor]

Resources seems like a good placement, but HAL team should be closely involved because they are the ones that can provide the best "hands on" requirements.

[RalfJung]

I was just made aware of this RFC and was quite surprised by parts of it. I hope this is an appropriate channel to ask such questions -- if not, please direct me elsewhere. :)

In particular, I am quite surprised by the argument of lock being &mut. According to my understanding of mutable references, that makes lock basically useless because it requires exclusive access to even call lock, at which point there cannot be any races anyway. You later show that the intent is to implement the Mutex API for a Copy type, so things are not actually exclusive, but that still leaves me puzzled.

1. For example, I think with this interface it is impossible to write a generic piece of code that takes a Mutex, spawns two threads, and uses the Mutex in both of them -- something like the following libstd+rayon code:

fn bump_concurrently(x: &Mutex<i32>) {
  rayon::join(
    || *x.lock().unwrap() += 1,
    || *x.lock().unwrap() += 1,
  );
}

Is that correct?

2. The RFC then shows that the Mutex trait is implemented for shared references, meaning the &mut Self is actually &mut &Mutex<...>. But then, why does that not entirely break the deadlock prevention scheme? Shouldn't the following work? (Code adjusted from the RFC)

static MUT: Mutex<RefCell<i32>> = Mutex::new(RefCell::new(0));

#[entry]
fn init() -> ! {
    let mut r = &MUT; // Note that r is mutable

    r.lock(|data| {
        let mut r2 = &MUT;
        r2.lock(|data| {/* oops, acquired the same lock twice. deadlock? */} );
    });
}

It seems deadlocks are only avoided for generic code that doesn't know that the type implementing core_mutex::Mutex is actually Copy. But there will always be some code that has to know the concrete type and hands out mutable references to all concurrently running operations.

3. And finally, even in generic code, there could be deadlocks due to lock ordering issues:

fn nest(
  mtx1: &mut impl core_mutex::Mutex<Data = i32>,
  mtx2: &mut impl core_mutex::Mutex<Data = i32>,
) {
  mtx1.lock(|data| mtx2.lock(|data| {}) )
}

// And now concurrently call `nest(mtx1, mtx2)` and `nest(mtx2, mtx1)`.

So, "deadlock-safe by construction" seems to be a best-effort property, but not a full guarantee? (With "by construction" I am expecting guarantees, like Rust code being "memory safe by construction".)

[Rahix]

For example, I think with this interface it is impossible to write a generic piece of code that takes a Mutex, spawns two threads, and uses the Mutex in both of them -- something like the following libstd+rayon code:
[...]
Is that correct?

Yes. This is a byproduct of the decisions which lead to this mutex design. For the full history, take a look at my original MR to embedded-hal and the RFC MR.

To summarize, the mutex design tries to abstract over a very broad range of 'things' which could be considered a mutex. This includes the more traditional mutex types (like the one in std) which is what you expected to find. But it also includes RTFM's mutex type which has entirely different semantics (can't be copied or moved across threads).

I tried to describe the differences in more detail in this comment.

But then, why does that not entirely break the deadlock prevention scheme?

It does. The deadlock prevention only works as long as the types are not Copy, as you noted. To battle this, I suggested adding a newtype wrapper (MutexRef) for this purpose (see this comment).

But IMO, the trait design does not help with deadlock prevention too much, it just needs to be this way to allow implementation for types like RTFM's mutex which do deadlock prevention.

And finally, even in generic code, there could be deadlocks due to lock ordering issues:
[...]
So, "deadlock-safe by construction" seems to be a best-effort property, but not a full guarantee?

Yes. As with your previous point, I don't think we gain deadlock prevention just from the mutex design, but from certain implementations and the traits need to be compatible with those ...

[RalfJung]

But IMO, the trait design does not help with deadlock prevention too much, it just needs to be this way to allow implementation for types like RTFM's mutex which do deadlock prevention.

Ah, that makes a lot of sense! Thanks for explaining that.

[korken89]

So, resources team for now then seems to be the consensus :)

[therealprof]

@korken89 So how do we get this repository started? Do you want me to create one?

[korken89]

@therealprof That would be great! I will then add implementation and then we can start reviewing it.

[therealprof]

https://github.com/rust-embedded/mutex-trait

And I assume you hit the close button by accident?

[korken89]

Ops, thanks for fixing!

[korken89]

I have added implementation and fixed the repo settings: https://github.com/rust-embedded/mutex-trait
Can people have a look so I did not goof something up (code and repo settings)? :)

[korken89]

Friendly reminder for review

[jonas-schievink]

I took a brief look at the crate:

• Do we want to commit to the 1.31+ MSRV policy? Which other WG crates use this policy? I'd be more in favor of a policy of supporting the last N Rust releases (usually N=3), since that allows using newer features without having to issue a breaking change.
• There's no impl for 1-tuples (this can be useful for macros, and the standard library does provide these)
• There's tuple impls for up to 16 elements, while the standard library only provides them for up to 12 elements. Why choose 16 here? Do we really need more than 12?
• There are no impls for types that are only in libstd, do we want to provide them behind a use-std feature? They can't be added by downstream crates (unless they use a newtype wrapper).
• It seems like the trait might make sense as part of bare-metal too, was this discussed anywhere? In general I think having fewer repos/crates reduces maintenance overhead. We can also reexport it there, which I think would be useful, but it's also the worst of both worlds (maintenance overhead is high, and now different teams have to coordinate as well).
  • This is also a question of whether we want to block publishing bare-metal 1.0 on a 1.0-quality Mutex trait. If not we can iterate in the mutex-trait crate until the trait is ready, then think about merging/reexporting from bare-metal.
• Do we want an Exclusive(&mut T) type that implements Mutex as a no-op, like RTFM has? It seems to me that using drivers that are generic over T: Mutex<Data = Bus> would be pretty painful without that.

Except the possible merge into bare-metal (if we decide to do that) I think it's time to release 0.1 of this crate ASAP, or nobody will start building things with it, which is really what we need to further evaluate the trait.

[jonas-schievink]

The "deadlock safe by construction" thing doesn't work out when the trait is implemented on immutable references by the way.

let a = core::cell::RefCell::new(0);

(&a).lock(|av| {
    *av += 1;

    (&a).lock(|_no| {});
});

It also forces one to use the (&a).lock syntax, which looks fairly alien.

[roblabla]

You can avoid writing (&a) by simply changing it to take the reference earlier, e.g.

let a = core::cell::RefCell::new(0);
let a = &a;

a.lock(|av| {
    *av += 1;

    a.lock(|_no| {});
});

[jonas-schievink]

Sure, but it still feels like an odd API

[jonas-schievink]

Most of my comments in #395 (comment) are addressed in rust-embedded/mutex-trait#5, should be ready for 0.1 soon ™️

[nickray]

Why are Deref and DerefMut not implemented for Exclusive? I guess locking with identity could work but... :)

[jonas-schievink]

Ah, missed those when adding the type. Opened rust-embedded/mutex-trait#6 to add them.

[adamgreig]

This is now live 🎉

I've set the resources team as the crate owner for now since I believe that was our conclusion at the time, but perhaps we can discuss that in the upcoming chat about teams.

[arjanmels]

I am looking to implement CriticalSection and multithreaded (CS+Spinlock) mutex for ESP32. I am wondering what the status of this RFC and its implementation is.

I noticed rust-embedded/mutex-trait#12, which mentions changes might be desirable. Is that something likely to happen soon?