[Idea] Spinlock with Interrupts Disabled #160

Shinribo commented 2024年02月21日 20:59:26 +01:00 (Migrated from github.com)

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.

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 commented 2024年02月21日 22:08:06 +01:00 (Migrated from github.com)

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.

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 commented 2024年02月22日 09:06:46 +01:00 (Migrated from github.com)

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.

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 commented 2024年02月22日 10:41:51 +01:00 (Migrated from github.com)

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.

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 commented 2024年02月22日 11:22:21 +01:00 (Migrated from github.com)

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

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

zesterer commented 2024年02月22日 16:47:08 +01:00 (Migrated from github.com)

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

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

Shinribo commented 2024年02月22日 17:15:51 +01:00 (Migrated from github.com)

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

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 commented 2024年02月22日 17:25:07 +01:00 (Migrated from github.com)

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.

> 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](https://github.com/rust-embedded/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 commented 2024年02月22日 17:33:00 +01:00 (Migrated from github.com)

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.

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 commented 2024年02月22日 17:46:56 +01:00 (Migrated from github.com)

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.

> 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 commented 2024年02月22日 17:52:09 +01:00 (Migrated from github.com)

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?

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 commented 2024年02月22日 18:53:46 +01:00 (Migrated from github.com)

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?

> 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](https://docs.rs/critical-section/latest/critical_section/fn.with.html) (or its single-core version which only uses crtical_section::with), am I misunderstanding some issue?

Shinribo commented 2024年02月22日 19:19:13 +01:00 (Migrated from github.com)

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.

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 commented 2024年02月22日 21:28:25 +01:00 (Migrated from github.com)

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.

`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 ```rs #[cfg(all(feature = "portable-atomic", portable_atomic_unsafe_assume_single_core))] ``` to avoid accidental use on multi-core systems.

Shinribo commented 2024年02月23日 08:37:14 +01:00 (Migrated from github.com)

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?

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 commented 2024年02月23日 14:02:09 +01:00 (Migrated from github.com)

@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.

@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 commented 2024年02月29日 14:51:16 +01:00 (Migrated from github.com)

@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.

@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 commented 2024年04月12日 21:04:10 +02:00 (Migrated from github.com)

@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 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 commented 2024年04月12日 23:23:33 +02:00 (Migrated from github.com)

@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!

> @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`!

qwandor commented 2024年08月06日 17:16:12 +02:00 (Migrated from github.com)

@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.

Would percore::ExceptionLock combined with a spin::Mutex do what you want? I think you need to mask exceptions before taking the spinlock to avoid potential deadlocks, so you could use ExceptionLock<Mutex<YourType>>.

> @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. Would [`percore::ExceptionLock`](https://docs.rs/percore/0.1.0/percore/struct.ExceptionLock.html) combined with a `spin::Mutex` do what you want? I think you need to mask exceptions before taking the spinlock to avoid potential deadlocks, so you could use `ExceptionLock<Mutex<YourType>>`.