Open kaspar030 opened 5 days ago
FYI, my recommendation (as a maintainer of portable-atomic) here is adding "portable-atomic?/require-cas"
to race
feature to have the appropriate error message displayed in cases such as the one @brodycj encountered, instead of always enabling the critical-section
feature of the portable-atomic
.
Relevant documentations are:
"Usage" section (readme, docs.rs):
If your crate supports no-std environment and requires atomic CAS, enabling the
require-cas
feature will allow theportable-atomic
to display a helpful error message to users on targets requiring additional action on the user side to provide atomic CAS.
critical-section feature in "Optional features" section (readme, docs.rs):
It is usually not recommended to always enable this feature in dependencies of the library.
Enabling this feature will prevent the end user from having the chance to take advantage of other (potentially) efficient implementations (Implementations provided by
unsafe-assume-single-core
feature, default implementations on MSP430 and AVR, implementation proposed in #60, etc. Other systems may also be supported in the future).The recommended approach for libraries is to leave it up to the end user whether or not to enable this feature. (However, it may make sense to enable this feature by default for libraries specific to a platform where other implementations are known not to work.)
portable-atomic
treatscritical-section
andunsafe-assume-single-core
as mutually exclusive.
By the way, the context on these being mutually exclusive is https://github.com/taiki-e/portable-atomic/pull/51 and https://github.com/taiki-e/portable-atomic/pull/94.
I suppose it is possible to resolve those priority issues by making critical-section
win and not be mutually exclusive, but in any case, here I would recommend making the critical-section
feature of portable-atomic
optional to allow for performance and code size optimization.
Thanks for headsup @kaspar030 and cc @brodycj
I guess I need to spend more time to understand what's going on here, but it looks like the latest release is in fact incorrect, so I will revert it in a couple of hours.
Yanked 1.20, as this is indeed a regression and is not something which should be happening in such a widely used crate.
Thanks!
Still, I think I am not understanding something:
once_cell
is using both critical_section
and portable_atomic
:
critical_section
is used as an ersatz-mutex to implement main types sync::OnceCell
portable_atomic
is used for, well, atomics, for racy race
typesIt sounds like you get in a situation where:
once_cell
with critical_section
enabledcritical_section
and portable atomicShouldn't portable-atomic in this case rely on critical section itself? I guess these are actually orthogonal:
portable_atomics
with unsafe-assume-single-core
. So that portable-atomics doesn't use critical_section
, but some other code does, and both are correct?If that is the case, than it seems once_cell
should actually have two public featurse:
critical_section
, that enables sync
moduleportable_atomic
, that enables race
moduleAnd that the users can enable either or both of this features.
Sorry for a rambling question, let me condencse this! Am I correct that the following situation is not a bug:
An embdded application is using portable-atomics
with unsafe-assume-single-core
feature enabled, but it also uses critical-section
and provides an interrupt-disabling implementaion of critical section?
@taiki-e I think your insight might be really helpful. My understanding is that we should try to offer as much flexibility as possible for higher-level librarians and applications that may have any combination of features enabled, or combination of multiple libraries that may have any combination of features enabled. For example: rustls currently uses once_cell, may or may not want to have critical-section enabled to support using portable-atomic with critical-section to help support building for thumbv6 ... rustls does currently use OnceBox for (via race?) no-std, maybe rustls should offer the "unsafe" single core option as well?
I am also starting to wonder if we should try to work more closely with rust-embedded to find an easier-to-understand way to get some of these options working consistently throughout multiple layers of libraries like this?
An embdded application is using
portable-atomics
withunsafe-assume-single-core
feature enabled, but it also usescritical-section
and provides an interrupt-disabling implementaion of critical section?
Correct, that's not a bug. portable-atomic
might provide the atomic operations via some single-core assuming shortcut (without disabling interrupts), while critical-section
might disable interrupts for (longer) critical sections. portable-atomic
might also just use critical sections though. It all depends. :)
I guess the fundamental thing here is that libraries should not make those choices, but leave them to the environment where they are used.
I am also starting to wonder if we should try to work more closely with rust-embedded to find an easier-to-understand way to get some of these options working consistently throughout multiple layers of libraries like this?
Actually the docs of both portable-atomic
and critical-section
are pretty clear for libraries - better don't select any options that select specific implementations.
It's just that most libraries turn into applications for their tests - there, choices need to be made ...
portable_atomic
, that enablesrace
module
IMO it should be the other way around. The race
feature should add the portable-atomic
dependency (as suggested by @taiki-e).
portable-atomic
already provides the equivalent of this, so at the price of an extra dependency, unconditionally using portable-atomic
would also just work everywhere (portable-atomic
passes through core::sync
atomics where possible).
Here is my understanding on the implementation before https://github.com/matklad/once_cell/pull/260 (i.e., 1.19.0):
First, there are two no-std compatible thread-safe implementations and each depends on different things:
once_cell::sync
: using critical-section
and atomic load/store (provided by portable-atomic
)
critical-section
is mandatory for this implementation.portable-atomic
is not mandatory for this implementation because atomic load/store is available in core
except for MSP430, PSX, and BPF targets.portable-atomic
is available as a dependency, it would be better to use portable-atomic
, because it can support the above targets that atomic load/store is not available in core
.once_cell::race
: using atomic CAS (provided by portable-atomic
if critical-section
feature is enabled)
critical-section
is not used for this implementation.portable-atomic
is not mandatory for this implementation because atomic CAS is available in core
except for thumbv6m, pre-v6 Arm, RISC-V without A extension, Xtensa, AVR, MSP430, MIPS PSX, and BPF targets.portable-atomic
is available as a dependency, it would be better to use portable-atomic
, because it can support the above targets that atomic CAS is not available in core
.And I think there are two issues:
portable-atomic
in once_cell::race
(to make it available on targets that don't support atomic CAS) requires critical-section
feature but it is confusing because critical-section
is not used at all in once_cell::race
.once_cell::race
requires atomic CAS, but portable-atomic
is not always able to provide it, and error messages when require-cas
feature of portable-atomic
is not enabled are confusing.For the first issue, it is better to allow using portable-atomic
without critical-section
feature.
I think there are two ways:
New Cargo feature to use portable-atomic
's atomic types instead of core
's:
[features]
race = ["portable-atomic?/require-cas"]
critical-section = ["dep:critical-section", "portable-atomic"]
[dependencies]
portable-atomic = { version = "1.3", optional = true, default-features = false }
#[cfg(not(feature = "portable-atomic"))]
use core::sync::atomic::AtomicPtr;
#[cfg(feature = "portable-atomic")]
use portable_atomic::AtomicPtr;
Target-specific dependency to use portable-atomic
's atomic types on cfg(not(target_has_atomic = "ptr"))
targets (no new Cargo feature):
[features]
race = ["dep:portable-atomic", "portable-atomic/require-cas"]
critical-section = ["dep:critical-section", "dep:portable-atomic"]
[target.'cfg(not(target_has_atomic = "ptr"))'.dependencies]
portable-atomic = { version = "1.3", optional = true, default-features = false }
#[cfg(target_has_atomic = "ptr")]
use core::sync::atomic::AtomicPtr;
#[cfg(not(target_has_atomic = "ptr"))]
use portable_atomic::AtomicPtr;
(Note that critical-section
feature needs to continue to have portable-atomic
enabled for compatibility with previous releases.)
For the second issue, adding require-cas
feature as above is better, but I have found that it is possible to display decent error messages without require-cas
feature using #[diagnostic::on_unimplemented]
stabilized in Rust 1.78: https://github.com/taiki-e/portable-atomic/issues/180
Before:
error[E0599]: no method named `compare_exchange` found for struct `portable_atomic::AtomicUsize` in the current scope
--> src/race.rs:60:24
|
60 | self.inner.compare_exchange(0, value.get(), Ordering::AcqRel, Ordering::Acquire);
| ^^^^^^^^^^^^^^^^ method not found in `AtomicUsize`
After:
error[E0277]: `compare_exchange` requires atomic CAS but not available on this target by default
--> src/race.rs:60:24
|
60 | self.inner.compare_exchange(0, value.get(), Ordering::AcqRel, Ordering::Acquire);
| ^^^^^^^^^^^^^^^^ this associated function is not available on this target by default
|
= help: the trait `HasCompareExchange` is not implemented for `&portable_atomic::AtomicUsize`
= note: consider enabling one of the `unsafe-assume-single-core` or `critical-section` Cargo features
= note: see <https://docs.rs/portable-atomic/latest/portable_atomic/#optional-features> for more.
note: required by a bound in `portable_atomic::AtomicUsize::compare_exchange`
--> /Users/taiki/projects/sources/taiki-e/portable-atomic/src/lib.rs:4064:5
|
4064 | atomic_int!(AtomicUsize, usize, 4);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| required by a bound in this associated function
| required by this bound in `AtomicUsize::compare_exchange`
= note: this error originates in the macro `atomic_int` (in Nightly builds, run with -Z macro-backtrace for more info)
@brodycj
maybe rustls should offer the "unsafe" single core option as well?
I don't think a feature for unsafe-assume-single-core
in rustls is needed. It has various associated features that control its behavior (annoying to forward all) and it can also be set globally via cfg.
260 propagates the
critical-section
feature to portable-atomics, which conflicts withunsafe-assume-single-core
.This breaks our use case.
portable-atomic
treatscritical-section
andunsafe-assume-single-core
as mutually exclusive. We depend on esp-rs, which for some platforms (e.g., esp32c2, c3) explicitly enables the latter.It is my understanding that
libraries
should not set eithercritical-section
orunsafe-assume-single-core
, but leave that to the end user.260 states this:
What does
critical-section
onportable-atomics
actually enable that onlycritical-section
feature enable? Maybe this can be solved inportable-atomic
with a backend-agnostic feature likerequire-cas
?