Add Cargo feature to use critical-section. #51

Dirbaio · 2022-12-14T23:47:45Z

I want to depreecate atomic-polyfill in favor of portable-atomic. This is the only thing left that portable-atomic can't do, so here it goes.

PR is still missing updating docs. I'll do it if you confirm you're onboard with the idea of supporting using critical-section.

notgull · 2022-12-15T00:39:56Z

It looks like you have some commits from #50 in your branch by accident.

I also wonder if a feature is the right way of doing this, since one of the things that portable-atomic wants to avoid is the "accidental feature activation" problem. However, the main alternative, having a --cfg option, also feels horribly unidiomatic, since you're using a --cfg option to import a dependency.

taiki-e · 2022-12-17T08:51:54Z

Thanks for the PR! I'm on board with merging this.

I think we need to decide which would win if both single-core cfg (portable_atomic_unsafe_assume_single_core) and critical-section feature were specified.
- Compile error because both are intended to be enabled by the end user.
- Or prefer single-core cfg because it is certain that the end user specified it.
Switching from the latter to the former would be a breaking change, and the opposite is probably not a breaking change, so it may be safe to adopt the former for now.
When critical-section feature is enabled, I believe IS_ALWAYS_LOCK_FREE (defined in the line above the with function) should be false. (At least on platforms that could be multi-core.)
When critical-section feature is enabled, I believe all operations including load/store must use CS, except for MSP430 (which is always single-core). Otherwise, on a multi-core system, atomic and non-atomic operations could occur concurrently, resulting in data races.
- Perhaps it would be easiest to replace cfg(target_arch = "avr")/cfg(not(target_arch = "avr")) in the impl Atomic { ... } with cfg(any(target_arch = "avr", all(feature = "..", not(target_arch = "msp430"))))/cfg(not(any(target_arch = "avr", all(feature = "..", not(target_arch = "msp430"))))).

I'll write more about feature vs cfg later, but I think it's probably fine to add this as a feature. (I'm still a bit torn about this though.)

taiki-e · 2022-12-17T09:18:40Z

As for build errors on non-embedded targets, this patch should fix it: d946bc2

Dirbaio · 2022-12-23T18:53:52Z

Thanks for the all-features fix! I was getting overwhelmed with so many cfg's :D

Compile error because both are intended to be enabled by the end user.

Done.

When critical-section feature is enabled, I believe IS_ALWAYS_LOCK_FREE should be false

Done.

re the last point: what should msp430 + critical-section do? Use critical-section, or keep using the msp430-specific impl ignoring critical-section?

Dirbaio · 2022-12-23T18:57:29Z

btw @taiki-e could you set GHA approval to "Require approval for first-time contributors who are new to GitHub" in repo settings? This way contributors don't have to wait for the GHA approval on their first PR.

Dirbaio · 2022-12-23T20:18:22Z

I've made it so critical-section is always used if requested, in any arch, including msp430.

It's necessary for soundness, since the critical-section contract is "if critical section is taken, no other critical section can run concurrently". Not "no other code can run concurrently". For example, a preemptive scheduler could preempt code that's within a critical section to run other (non-critical-section) code. Even on single core.

taiki-e · 2022-12-28T15:04:59Z

btw @taiki-e could you set GHA approval to "Require approval for first-time contributors who are new to GitHub" in repo settings? This way contributors don't have to wait for the GHA approval on their first PR.

Done.

I've made it so critical-section is always used if requested, in any arch, including msp430.

It's necessary for soundness, since the critical-section contract is "if critical section is taken, no other critical section can run concurrently". Not "no other code can run concurrently". For example, a preemptive scheduler could preempt code that's within a critical section to run other (non-critical-section) code. Even on single core.

What we wanted to guarantee on MSP430 was the behavior of not disabling interrupts in load/store, add/sub/and/or/xor (added by #47), not (added by #54), etc. (See also msp430-atomic's readme)

However, I did not know such behavior was incompatible with critical-section (cc @cr1901). There is a user who depends on these guarantees, so if these guarantees cannot be provided with critical-section, I think we will need to disable critical-section support on MSP430.

Dirbaio · 2022-12-28T15:58:33Z

critical-section implementations are not necessarily "disable interrupts", they could be other things.

For example, one valid implementation is locking a global RTOS mutex. The RTOS is allowed to preempt a task that's holding the mutex, it's sound because no other task can acquire the mutex concurrently (if it tries to, the RTOS would deschedule it, reschedule the original task to let it finish, then carry on).

If we impl add with msp430 asm and fetch_add with critical-section, the following could happen:

Thread A: acquires CS mutex.
Thread A: starts doing a fetch_add, does the read
Thread A: gets preempted
Thread B: does an add. The RTOS lets it because it doesn't acquire the CS mutex.
Thread B: gets preempted
Thread A: does the add and the write
Thread A: releasees the CS mutex.

I don't know how common it is to use RTOSs on MSP430, and how common would it be to want to use a RTOS mutex instead of disabling interrupts (I imagine it'd be slower). I don't think we should forbid using c-s on msp430 though. Supporting it is no extra effort, it's the same code as other archs. The operations you mention are still guaranteed to not disable interrupts on msp430 if you don't enable c-s support.

cr1901 · 2022-12-28T16:17:09Z

except for MSP430 (which is always single-core)

AVR?

However, I did not know such behavior was incompatible with critical-section

I think the idea for a preemptive critical-section implementation on MSP430/AVR is that one thread could be executing a critical section, be preempted, and then another thread could do an atomic load/store, or add, sub, xor, etc to the memory location that was being modified in the critical section of the first thread. Since none of these other operations require a critical section to execute in portable-atomic, this is a data race.

@Dirbaio's solution to "unconditionally use critical sections for all operations" is a workaround. But I still want the optimizations.¹

I think if the critical-section feature is enabled, perhaps there should be another unsafe config that's a companion to portable_atomic_unsafe_assume_single_core. Such a feature would gate optimizations that allow some operations on portable-atomic types to complete without a critical-section. Maybe something like portable_atomic_unsafe_assume_cs_always_succeeds.

For instance, the critical section implementation of msp430 crate disables interrupts. This is functionally identical to the msp430-specific impl in portable-atomic. Therefore, the critical-section feature can coexist with the functions of portable-atomic that don't disable interrupts if you know the critical-section implementation you're using always succeeds (e.g. is wait-free) in acquire/release.

I don't think we should forbid using c-s on msp430 though.

Agreed, FWIW.

Also perhaps it could be documented better that portable_atomic_unsafe_assume_single_core doesn't disable interrupts if it doesn't have to, such as for atomic loads/stores and add, sub, etc?

Dirbaio · 2022-12-28T16:33:59Z

Why is it an issue to disable the optimizations when c-s is enabled? If the user wants these optimizations, they can leave the critical-section feature disabled.

if we document the optimization, and the fact that the optimizations are gone if you enable c-s, and give a guideline like "if all your c-s implementation does is disable interrupts (which is the most likely), there's no advantage in enabling c-s, keep it disabled so you can keep the optimizations", then it shouldn't be a big issue IMO.

cr1901 · 2022-12-28T23:00:02Z

Why is it an issue to disable the optimizations when c-s is enabled? If the user wants these optimizations, they can leave the critical-section feature disabled.

Dependencies may enable the critical-section feature, and thanks to feature union, you leaving the feature disabled in your application will not be honored.

if we document the optimization, and the fact that the optimizations are gone if you enable c-s, and give a guideline like "if all your c-s implementation does is disable interrupts (which is the most likely), there's no advantage in enabling c-s, keep it disabled so you can keep the optimizations", then it shouldn't be a big issue IMO.

That said, I'm fine with this. Hopefully not many downstream crates from portable-atomic (but upstream from your shiny new app) will enable the critical-section feature unconditionally.

korken89 · 2023-01-05T06:59:08Z

How's this going?
I was looking to go away from atomic-polyfill for heapless, but I'd like to see this merged and released first.

taiki-e · 2023-01-05T14:23:26Z

@korken89

If we can agree on a tentative direction on the platforms for which we are providing atomic implementations by default (MSP430 and AVR), I think we can move forward with this.

(No need to worry about CI failures and the above platform handling, as these can be taken care of on my end before merging if needed. )

@Dirbaio

I don't think we should forbid using c-s on msp430 though. Supporting it is no extra effort, it's the same code as other archs. The operations you mention are still guaranteed to not disable interrupts on msp430 if you don't enable c-s support.

The problem is that (if we do this) enabling the critical-section feature drops the guarantee provided by default¹.
That would violate the design principle of the cargo feature that "features should be additive", and would be a problem if the user relied on that guarantee to ensure soundness. ²

As far as I know, a sound solution to that problem is one of the following:

Require portable_atomic_unsafe_assume_single_core cfg even if the architecture is always single core (i.e., on MSP430/AVR). (breaking change)
Do not support critical-section (i.e., deny or ignore it) and always use default implementations on architectures that always single core.
Require cfg to enable critical-section support on architectures that we provide default implementations.

I had the second thing in mind when I first commented, but the third is also fine.

Also even if you want to allow them eventually, we can start with deny for now and discuss it in a follow-up issue. (since deny->allow is not a breaking change)

@cr1901

except for MSP430 (which is always single-core)

AVR?

Yeah, AVR is also always single-core, but that comment was about optimizations such as load/store, and only MSP430 needed to mention that optimization since AVR always disables interrupts even in load/store.

I think if the critical-section feature is enabled, perhaps there should be another unsafe config that's a companion to portable_atomic_unsafe_assume_single_core. Such a feature would gate optimizations that allow some operations on portable-atomic types to complete without a critical-section. Maybe something like portable_atomic_unsafe_assume_cs_always_succeeds.

I think such a cfg would make sense, but I would like to discuss this in a follow-up issue as well.

Also perhaps it could be documented better that portable_atomic_unsafe_assume_single_core doesn't disable interrupts if it doesn't have to, such as for atomic loads/stores and add, sub, etc?

The methods that benefit from such optimization have documentation describing them (except for load/store), but it seems to make sense to explain them comprehensively in the interrupt module's readme.

We provide interrupt-disabling based implementations by default on targets that are always single-core. https://github.com/taiki-e/portable-atomic/pull/31 As for AVR, this implementation is also consistent with the atomic implementation of LLVM that disable interrupts. ↩
I don't expect end users to rely on that guarantee while also enabling critical-section feature, but what about the case where the author of the library relies on this guarantee and the end user is unaware of it and enables the critical-section feature/implementation? That seems like a pitfall to me. ↩

Dirbaio · 2023-01-05T14:40:14Z

The problem is that (if we do this) enabling the critical-section feature drops the guarantee provided by default1.
That would violate the design principle of the cargo feature that "features should be additive", and would be a problem if the user relied on that guarantee to ensure soundness. 2

Can you give an example of code that would rely on that guarantee for soundness? The semantics of the API are the same with both the msp430-specific and the critical-section implementations. I don't see how some piece of code could be sound with the first and not with the second.

taiki-e · 2023-01-05T15:41:52Z

Can you give an example of code that would rely on that guarantee for soundness?

The specific use case I know of was the one discussed in the msp430-atomic issue. (However, I have not confirmed to see if that would cause a soundness issue.)

The semantics of the API are the same with both the msp430-specific and the critical-section implementations. I don't see how some piece of code could be sound with the first and not with the second.

I believe the semantics of with are the same, but I think the fact that some methods are guaranteed not to be used with in the default implementation could be problematic.

That said, I'm not an MSP430 expert, so if @cr1901 or/and @YuhanLiin are sure that calling interrupt-related instructions (or something used in the critical-section implementation) in contexts where interrupt-related instructions should not be called will not cause soundness issues, I'm in favor of supporting the critical-section feature as usual on those platforms.

Dirbaio · 2023-01-05T15:55:33Z

The specific use case I know of was pftbest/msp430-atomic#7 (comment). (However, I have not confirmed to see if that would cause a soundness issue.)

I don't think that causes a soundness issue, it's just slower. The add instruction does a read then a write on the bus anyway. The peripheral can't even know if these were caused by an add or a load then a store.

I believe the semantics of with are the same, but I think the fact that some methods are guaranteed not to be used with in the default implementation could be problematic.

I mean the semantics of the actual public API. For example: the semantics of .add() are "increments the value, is guaranteed to not race with other accesses to the same instance, causes the right memory barriers depending on the Ordering".

The HOW it does that (disabling irqs, locking a RTOS mutex, whatever) is not part of the semantics, it's an implementation detail. All implementations we have (use the add msp430 instruction, or disable irqs, or acquire critical-section) satisfy these semantics, therefore they're interchangeable.

taiki-e · 2023-01-11T16:56:08Z

I mean the semantics of the actual public API. For example: the semantics of .add() are "increments the value, is guaranteed to not race with other accesses to the same instance, causes the right memory barriers depending on the Ordering".

The HOW it does that (disabling irqs, locking a RTOS mutex, whatever) is not part of the semantics, it's an implementation detail. All implementations we have (use the add msp430 instruction, or disable irqs, or acquire critical-section) satisfy these semantics, therefore they're interchangeable.

Thanks for the clarification. Some implementation details in the interrupt module may be treated like the semantics of the public API (e.g., if it affects the soundness). That said, it should affect the soundness only on platforms that require opt-in via single-core cfg, so I think it indeed makes sense to treat it as implementation details on MSP430 and AVR.

taiki-e · 2023-01-11T19:19:44Z

So, I now think that the current implementation is (probably) fine as it is.

And, I think the remaining things needed to merge this are docs and CI fixes.

Could you add docs to "Optional features" section?
As for the failure of test (nightly, windows-latest), it is an upstream bug, but there is a workaround in the main branch, so the rebase should fix the problem.
Most of the remaining failures should be fixed with this patch: fe91708
As for the failure of test (1.59), std critical-section implementation compatible with Rust 1.59 is needed. Something like: a122e95

- Update cspell dictionary - Exclude critical-section feature in MSRV check - Exclude critical-section feature in pre-1.54 build - Exclude critical-section feature in build with single-core cfg - Reduce memory usage in miri test by reducing QUICKCHECK_TESTS - Allow dead_code in imp::{msp430,riscv} modules when critical-section feature enabled - Fix cfg for riscv without atomics

Dirbaio · 2023-01-11T20:35:50Z

@taiki-e thanks for the CI fixes! I've pulled those in. I've also added docs to the README.

README.md

taiki-e

LGTM. Thanks @Dirbaio!

bors r+

taiki-e · 2023-01-12T03:00:08Z

Opened #61 to track the new release includes this. I think I can release this during this weekend.

bors · 2023-01-12T04:04:14Z

Build succeeded:

taiki-e · 2023-01-15T07:29:27Z

Published in portable-atomic v1.0.0.

Dirbaio force-pushed the critical-section branch from c3c1dea to 094b354 Compare December 15, 2022 00:06

Dirbaio force-pushed the critical-section branch from 094b354 to de040ba Compare December 15, 2022 11:41

Dirbaio mentioned this pull request Dec 23, 2022

Use portable-atomic instead of atomic-polyfill. rust-embedded/heapless#328

Merged

Dirbaio force-pushed the critical-section branch from 96984d5 to 0077d2e Compare December 23, 2022 20:14

Dirbaio marked this pull request as ready for review December 23, 2022 20:18

This was referenced Jan 5, 2023

Optional (unsafe) cfg to use atomic builtins on targets where atomic CAS is not supported, or on atomic operations of unsupported sizes #33

Closed

Add cfg to use __atomic_* builtins #30

Closed

Dirbaio and others added 5 commits January 11, 2023 20:38

WIP: add feature to use critical-section.

6ef0404

fix all features

99475a0

Disallow single-core plus critical-section enabled simultaneously.

891f19a

Do not consider critical-section impls lock-free.

fc29724

if critical-section is enabled, always use it regardless of arch.

cbfbc1d

taiki-e added 2 commits January 11, 2023 20:38

Use our own critical-section impl compatible with Rust 1.59

6dda277

Dirbaio force-pushed the critical-section branch from 0077d2e to 6dda277 Compare January 11, 2023 19:39

Dirbaio changed the title ~~WIP: add feature to use critical-section.~~ Add Cargo feature to use critical-section. Jan 11, 2023

taiki-e reviewed Jan 12, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

Dirbaio added 2 commits January 12, 2023 03:04

Document critical-section feature in README.

b97e51c

Update docs for interrupt module.

8b4a183

Dirbaio force-pushed the critical-section branch from f4f5629 to 8b4a183 Compare January 12, 2023 02:04

taiki-e approved these changes Jan 12, 2023

View reviewed changes

taiki-e mentioned this pull request Jan 12, 2023

Tracking issue: release 1.0.0 that includes critical-section support #61

Closed

3 tasks

bors bot merged commit 008b5a2 into taiki-e:main Jan 12, 2023

taiki-e mentioned this pull request Feb 20, 2023

Update portable-atomic to 1 mvdnes/spin-rs#139

Merged

ia0 mentioned this pull request Apr 24, 2023

Support architectures without atomics with polyfill rust-embedded-community/usb-device#115

Merged

taiki-e mentioned this pull request Sep 16, 2024

regression with portable-atomics in 1.20.0 matklad/once_cell#264

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Cargo feature to use critical-section. #51

Add Cargo feature to use critical-section. #51

Dirbaio commented Dec 14, 2022 •

edited

Loading

notgull commented Dec 15, 2022

taiki-e commented Dec 17, 2022 •

edited

Loading

taiki-e commented Dec 17, 2022 •

edited

Loading

Dirbaio commented Dec 23, 2022

Dirbaio commented Dec 23, 2022

Dirbaio commented Dec 23, 2022

taiki-e commented Dec 28, 2022 •

edited

Loading

Dirbaio commented Dec 28, 2022

cr1901 commented Dec 28, 2022 •

edited

Loading

Dirbaio commented Dec 28, 2022 •

edited

Loading

cr1901 commented Dec 28, 2022 •

edited

Loading

korken89 commented Jan 5, 2023

taiki-e commented Jan 5, 2023

Dirbaio commented Jan 5, 2023

taiki-e commented Jan 5, 2023 •

edited

Loading

Dirbaio commented Jan 5, 2023

taiki-e commented Jan 11, 2023

taiki-e commented Jan 11, 2023

Dirbaio commented Jan 11, 2023

taiki-e left a comment

taiki-e commented Jan 12, 2023

bors bot commented Jan 12, 2023

taiki-e commented Jan 15, 2023

Add Cargo feature to use critical-section. #51

Add Cargo feature to use critical-section. #51

Conversation

Dirbaio commented Dec 14, 2022 • edited Loading

notgull commented Dec 15, 2022

taiki-e commented Dec 17, 2022 • edited Loading

taiki-e commented Dec 17, 2022 • edited Loading

Dirbaio commented Dec 23, 2022

Dirbaio commented Dec 23, 2022

Dirbaio commented Dec 23, 2022

taiki-e commented Dec 28, 2022 • edited Loading

Dirbaio commented Dec 28, 2022

cr1901 commented Dec 28, 2022 • edited Loading

Dirbaio commented Dec 28, 2022 • edited Loading

cr1901 commented Dec 28, 2022 • edited Loading

korken89 commented Jan 5, 2023

taiki-e commented Jan 5, 2023

Footnotes

Dirbaio commented Jan 5, 2023

taiki-e commented Jan 5, 2023 • edited Loading

Dirbaio commented Jan 5, 2023

taiki-e commented Jan 11, 2023

taiki-e commented Jan 11, 2023

Dirbaio commented Jan 11, 2023

taiki-e left a comment

Choose a reason for hiding this comment

taiki-e commented Jan 12, 2023

bors bot commented Jan 12, 2023

taiki-e commented Jan 15, 2023

Dirbaio commented Dec 14, 2022 •

edited

Loading

taiki-e commented Dec 17, 2022 •

edited

Loading

taiki-e commented Dec 17, 2022 •

edited

Loading

taiki-e commented Dec 28, 2022 •

edited

Loading

cr1901 commented Dec 28, 2022 •

edited

Loading

Dirbaio commented Dec 28, 2022 •

edited

Loading

cr1901 commented Dec 28, 2022 •

edited

Loading

taiki-e commented Jan 5, 2023 •

edited

Loading