matklad/once_cell

regression with portable-atomics in `1.20.0`

Closed this issue · 10 comments

kaspar030 commented

#260 propagates the critical-section feature to portable-atomics, which conflicts with unsafe-assume-single-core.

  • propagate critical-section feature selection into portable-atomic

This breaks our use case. portable-atomic treats critical-section and unsafe-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 either critical-section or unsafe-assume-single-core, but leave that to the end user.

#260 states this:

ORIGINAL RATIONALE RE: propagate critical-section feature selection into portable-atomic

Normally just using critical-section should be good enough for targets like super-outdated thumbv6m-none-eabi target. But in https://github.com/rustls/rustls/pull/2088 I would like to preserve existing use of alloc option, in which case I had to add explicit portable-atomic dependency with its critical-section feature enabled.

IMO it would be nice if I didn't have to add explicit sub-depencencies with this option. This is my proposal to enable critical-section for portable-atomic if portable-atomic is wanted.

Please let me know if you have any other idea or perspective concerning this.

What does critical-section on portable-atomics actually enable that only critical-section feature enable? Maybe this can be solved in portable-atomic with a backend-agnostic feature like require-cas?

taiki-e commented

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 the portable-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 treats critical-section and unsafe-assume-single-core as mutually exclusive.

By the way, the context on these being mutually exclusive is taiki-e/portable-atomic#51 and taiki-e/portable-atomic#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.

  • ❤️1
matklad commented

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.

matklad commented

Yanked 1.20, as this is indeed a regression and is not something which should be happening in such a widely used crate.

kaspar030 commented

Thanks!

  • 👍1
matklad commented

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 types

It sounds like you get in a situation where:

Shouldn't portable-atomic in this case rely on critical section itself? I guess these are actually orthogonal:

  • You need portable atomics for actual atomicity. If you know you have a single core only, you can avoid atomic operations
  • You need critical section for non-atomic blocking operations. Basically, to prevent interrupts from messing things up
  • So it might be the case that, on a single core, you need critical section, but you could still use 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 module
  • portable_atomic, that enables race module

And that the users can enable either or both of this features.

matklad commented

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?

brodycj commented

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

kaspar030 commented

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?

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

kaspar030 commented
  • portable_atomic, that enables race 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).

taiki-e commented

Here is my understanding on the implementation before #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.
    • If 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.
    • If 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:

  • Using 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: taiki-e/portable-atomic#180
UPDATE: this improvement is released in portable-atomic 1.8.0.

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