[Idea] Spinlock with Interrupts Disabled

[Shinribo]

The idea is to have the spin lock disable interrupts on entry and restore them to their former state when leaving the locked region. This would make it easier to prevent accidentally task switch with a active spinlock without adding extensive overhead or investing a lot of brainpower when writing code.

[zesterer]

This would not be safe in the general case, and could only be made to work on single-core systems. We'd probably want to implement it as a distinct variant of mutex (akin to FairMutex and TicketMutex), but only enabled according to the rules laid out by portable-atomic.

[Shinribo]

Adding a new Mutex instead of changing the expected behavior of the currently used Mutex makes sense. However i dont understand how disabling interrupts when the lock is held would be unsafe as interruption of a spinlock would lead to unexpected deadlocks while poor performance caused by long operations with the NoInterruptMutex locked would be safe behavior that's also easier to spot and debug than pseudorandom deadlocks. It would make sense to shortly restore interrupts and then disable them again when aquiring the spinlock fails to allow rescheduling while spinning.

[zesterer]

Use of the current mutex implementations on single-core platforms with no atomic instructions is safe, but - as with any architecture - locking/blocking/waiting in an interrupt handler is a possible source of deadlocks.

Disabling interrupts alone on a multi-core system is unsafe because interrupt disabling is generally core-specific, so it's quite possible for some other core to come along and trample all over the state being manipulated by another core.

So, IrqMutex would only be useful in cases where the mutex is accessed from an interrupt (a regular RTOS-like design could just use a regular Mutex, which would work fine for multiple threads, even on a single-core architecture), but would only be safe in cases where the architecture has only a single core.

[Shinribo]

So, will IrqMutex be added to the crate in the forseeable future?

[zesterer]

A difficulty is finding a portable way to enable/disable interrupts. Sadly, portable-atomic doesn't expose anything of the sort publicly.

[Shinribo]

I dont see any portable way to do it unless conditional compiling is used and for every supported ISA two small inline? assembly stubs are used. My idea would be to support the ~3 most used ISAs and create a trait to allow users to create enable/disable interrupt function for unsupported ISAs or a similar mechanism

[taiki-e]

A difficulty is finding a portable way to enable/disable interrupts. Sadly, portable-atomic doesn't expose anything of the sort publicly.

AFAIK critical-section crate is often used in rust-embedded and its ecosystem for this purpose. (portable-atomic also supports it as an option.) The one that portable-atomic uses internally is optimized for its use, so it would be difficult to expose.

[Shinribo]

i currenlty dont see how this would interfere with calling a enable/disable interrupts function before and after some atomic operations. But maybe im thinking the wrong way.

For example if the IrqMutex would require a struct that implements a InterruptControl Trait (enable(), disable(), set(bool)) and then calling those functions before/after a given aquire/release functions.

[taiki-e]

enable/disable interrupts

To be clear: Both critical-section and portable-atomic actually implement (or require the user to implement) this as disable-interrupts/restore-interrupts-state, not disable-interrupts/enable-interrupts. This is one of the requirements for proper handling of nesting.

[Shinribo]

So if i now understand correctly that while portable-atomic allows my idea the problem is that its difficulty to write the spin-rs IrqMutex implementation to pass throu the user implementation?

[taiki-e]

the problem is that its difficulty to write the spin-rs IrqMutex implementation to pass throu the user implementation

Um, I'm sorry, I thought you needed the equivalent of using spin::Mutex within the scope of crtical_section::with (or its single-core version which only uses crtical_section::with), am I misunderstanding some issue?

[Shinribo]

My initial question was if it is possible to add a Mutex that also disables interrupts on the core currently holding the lock and the response was that its complicated and now i just try to understand why its more complicated than just adding two function calls (disable() -> bool, restore(bool)) and a trait (to allow the Mutex to be Hardware Agnostic) to create a IrqMutex.

[zesterer]

critical-section should do the job, yes. I'll see if I can find the time to work on this later.

If anybody is interested in implementing this, the best approach would probably be to copy the implementation of SpinMutex, but swap out the atomic boolean with critical_section::acquire/critical_section::release. The module should be guarded by something like

#[cfg(all(feature = "portable-atomic", portable_atomic_unsafe_assume_single_core))]

to avoid accidental use on multi-core systems.

[Shinribo]

Thanks for adding this feature, but my intention was to get a IrqMutex that is also multi-core safe, is there no way to implement that?

[le-jzr]

@zesterer I think you're misunderstanding the question. The point is not to implement mutex via disabling interrupts, but rather to implement mutex that's a fully multicore safe spinlock that additionally disables interrupts on the local core.

This is a common construct inside kernels, since interrupt inside a spinlock critical section creates both a contention issue and often a deadlock hazard.

[Shinribo]

@zesterer did you already started to implement IrqMutex like @le-jzr described it or can i try to do it? Also we might need a better name for it.

[Shinribo]

@zesterer I wrote the code for the IrqMutex, should i create a PR to the main branch or do you want to create a new branch?

[zesterer]

@zesterer I wrote the code for the IrqMutex, should i create a PR to the main branch or do you want to create a new branch?

Feel free to open a PR on main!