Theseus icon indicating copy to clipboard operation
Theseus copied to clipboard

Implement `raw_mutex` and `mutex`

Open tsoutsman opened this issue 3 years ago • 2 comments

The current implementation isn't particularly performant, but it is correct. Improving the performance of the mutex is something we could do in a later PR.

Unlike mutex_sleep, raw_mutex lock operations panic rather than return an error if task::get_my_current_task() fails. This conforms to the std::sys mutex API where operations mustn't fail.

mutex::Mutex is a copy-paste of std::sync::Mutex except it doesn't keep track of poisoning. As a result, it's lock and unlock operations are also guaranteed to suceed.

I haven't removed mutex_sleep yet because it also implements RwLock which I'll add in a later PR.

Signed-off-by: Klim Tsoutsman [email protected]

tsoutsman avatar Jul 26 '22 00:07 tsoutsman

Thanks for the PR, and sorry it took me so long to review it. I've got a couple thoughts/questions first; we can also discuss this on Discord.

To clarify, this new Mutex type is what std would use in its platform-specific lower layers in order to provide std::sync::Mutex? If so, I'm not sure why we'd need an entirely new Mutex implementation to provide that, since we could just use any of the existing Mutexes (Theseus-specific or otherwise) to provide that sync primitive.

Also, I'm not sure about exposing an interrupt-disabling Mutex for use within std, that seems like a very heavyweight approach and also likely overly cautious. Perhaps it's needed, but off the top of my head I can't think of any scenarios where data would be shared between a std component and an interrupt handler -- surely everything required for interrupt handling would be well hidden from std. But my largest concern therein is that if a user of std obtained and subsequently called mem::forget on a MutexGuard, then interrupts would be disabled permanently on that CPU. IMO, there shouldn't be any other side effects of a forgotten mutex other than that specific shared data item being locked for good.

kevinaboos avatar Aug 16 '22 17:08 kevinaboos

To clarify, this new Mutex type is what std would use in its platform-specific lower layers in order to provide std::sync::Mutex? If so, I'm not sure why we'd need an entirely new Mutex implementation to provide that, since we could just use any of the existing Mutexes (Theseus-specific or otherwise) to provide that sync primitive.

raw_mutex::Mutex is. mutex::Mutex is just a copy of std::sync::Mutex so we can use the nice interface without relying on std. No kernel crate other than mutex (and raw_rwlock/raw_condvar) should directly depend on raw_mutex. They should all use mutex.

Also, I'm not sure about exposing an interrupt-disabling Mutex for use within std, that seems like a very heavyweight approach and also likely overly cautious.

It probably doesn't need to disable interrupts, just preemption. I made this PR before #595 but this is something that we can do now. For reference Linux disables preemption (search "preemption").

But my largest concern therein is that if a user of std obtained and subsequently called mem::forget on a MutexGuard, then interrupts would be disabled permanently on that CPU.

The inner interrupt-disabling (or preemption-disabling) spin lock is only locked when acquiring or releasing the mutex (i.e. only in Mutex::lock and MutexGuard::drop). Interrupts are enabled even if you hold a MutexGuard.

Something to note is mutexes used by interrupts should still just be IRQ-safe spin locks.

tsoutsman avatar Aug 18 '22 09:08 tsoutsman