Use portable-atomic instead of atomic-polyfill.

[Dirbaio]

I intend to deprecate atomic-polyfill soon, in favor of portable-atomic. It supports more archs and is more actively maintained. The only thing it can't do yet is use critical-section, but it should soon it already can: taiki-e/portable-atomic#51

[korken89]

Hi please rebase to fix tsan failure

[korken89]

Hi, I gave this a check and saw something weird.

RUSTFLAGS="--cfg=portable_atomic_unsafe_assume_single_core"

Is there a reason this is not a feature to heapless?

[Dirbaio]

it's a feature for atomic-polyfill. See its readme. The reasoning for making it a --cfg and not a Cargo feature is that enabling it on non-multicore chips can cause unsoundness, so the author wants to enforce only the final binary crate can enable it, not library crates. (it's a decision I don't agree with, see some discussion here... 🤷 )

[korken89]

Hmm, I see - that was one long discussion. 😅

Not sure what I think of it yet, I'll leave this open until I can come to terms with --cfg or if @japaric forces it through.

[Dirbaio]

Pushed new version:

• Rebase
• Use portable-atomic 1.0
• Use portable-atomic/critical-section feature in CI tests, instead of the --cfg.

With the current implementation, for targets without native CAS, the end-user will have to add a dep on portable-atomic to configure it. Either enabling the critical-section feature, or the evil --cfg=portable_atomic_unsafe_assume_single_core. Not sure if that's OK, or how we can do better.

[taiki-e]

It seems that this SPSC implementation does not require atomic CAS and only uses atomic load/store.

(FWIW, portable-atomic provides multicore-safe atomic load/store for riscv without A extension, even without critical-section feature or unsafe single-core cfg)

[taiki-e]

At this time, a builtin AVR target (avr-unknown-gnu-atmega328) does not provide atomic, and AVR targets used in avr-hal do not provide atomic load-store of pointer width. So I guess spsc which uses AtomicUsize will always require portable-atomic.

However, LLVM can provide 16-bit atomic load/store for AVR, so there is no need to necessarily remove optional=true here. (I should send a patch to rustc/avr-hal to update the target specs. EDIT: opened rust-lang/rust#114495)

[taiki-e]

With the current implementation, for targets without native CAS, the end-user will have to add a dep on portable-atomic to configure it. Either enabling the critical-section feature, or the evil --cfg=portable_atomic_unsafe_assume_single_core. Not sure if that's OK, or how we can do better.

When using the critical-section feature, I think what the user needs to do is the same as when using any other crates that have critical-section feature, such as once_cell.
So I'm not sure if that is the best, but I think it is fine.

In any case, I hope examples I just added clarifies the step to use them...

(Regarding whether to use cfg or feature, I'm considering adding a comment something like "If you feel that using the single-core cfg is annoying, consider using the critical-section feature"...)

[Dirbaio]

@taiki-e: There's many cases where a crate knows it's going to run in single-core (for example embassy-stm32 with stm32f4xx feature. All F4 chips are single-core). In these cases it'd want to enable the atomic-polyfill/unsafe-assume-single-core Cargo feature if it existed, so that it all Just Works for the end user.

Asking the user to add nonstandard --cfg args is very annoying. Using the critical-section feature to avoid it is suboptimal, it'll result in worse performance unnecessarily (it has to acquire the CS for load/store, while unsafe-assume-single-core only has to for CAS ops).

Could you please consider adding a unsafe-assume-single-core Cargo feature?

[taiki-e]

unsafe-assume-single-core Cargo feature

After discussion in tokio-rs/bytes#573, I'm starting to think that adding per-target/implementation unsafe-assume-single-core-* features (e.g., unsafe-assume-single-core-cortex-m) might be fine.

The cross-platform unsafe-assume-single-core feature can be roughly enabled by the user, and also cargo v1 resolver may enable it for unintended targets, so I'm not positive to add it.

However, I think per-target/implementation unsafe-assume-single-core-* features that cause compile error on unsupported targets might be enough (though not perfect¹) to prevent such an accidental misuse (cross-platform unsafe-assume-single-core feature silently causes problems). (I have previously seen a GUI-related crate that causes a compile error and suggests the use of cargo v2 resolver, when the feature is accidentally enabled on unsupported targets.)

Footnotes

1. It does not address all of my original concerns, but I think it can address the most annoying part " even if the library maintainer understands the target correctly and takes the correct action, (and the end user takes the correct action) it may not work correctly for the end user". ↩

[Dirbaio]

ping @korken89

atomic-polyfill is now deprecated, and portable-atomic 1.4 added the unsafe-assume-single-core Cargo feature so now there's no case where you need --cfgs in embedded usage.