Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an atomic-polyfill optional dependancy #165

Merged
merged 2 commits into from Dec 14, 2021
Merged

Add an atomic-polyfill optional dependancy #165

merged 2 commits into from Dec 14, 2021

Conversation

ogoffart
Copy link
Contributor

@ogoffart ogoffart commented Dec 1, 2021

To make it compile on platform that have limited atomics

Fixes #155

Note that this re-implement the Debug version of the OnceBox
because atomic-polyfill::AtomicPtr don't implement Debug (embassy-rs/atomic-polyfill#11)

To make it compile on platform that have limited atomics

Note that this re-implement the Debug version of the OnceBox
because atomic-polyfill::AtomicPtr don't implement Debug
@ogoffart
Copy link
Contributor Author

Ping?

@matklad
Copy link
Owner

matklad commented Dec 14, 2021

Sorry, didn't get the chance to look into this closely.

On the first blush, I am unsure about this -- I am not a huge fan of the solutions in the style of "silently replace mutexes with spinlocks" -- I generally thing the caller should be responsible for selecting the right synchronization primitive.

This case looks like it might actually be OK though? What this does is disabling interrupts for the duration of the CAS, and that should be OK, as we are not calling any user-supplied code within the scope of critical section? What happens if there are multiple cores/CPUs in play)? Or are we sure that that that's not a problem for these targets?

I guess, given that this is a disabled by default features, it's not that important. Could you bump a version in Cargo.toml as well, such that this gets a release automatically when merged?

@ogoffart
Copy link
Contributor Author

The use-case is indeed to be able to make cross platform libraries that use once_cell::race so they can be used on both platform with a std library, and also on bare metal embedded on device that do not have threads and atomic.

I am not a huge fan of the solutions in the style of "silently replace mutexes with spinlocks" -- I generally thing the caller should be responsible for selecting the right synchronization primitive.

I have read https://matklad.github.io/2020/01/02/spinlocks-considered-harmful.html and I agree 100%
But in this case, there is probably no spinlock involved, even when this feature is enabled
If this is a platform that supports atomic, then the core::sync::atomic classes are used so no change at all.
If the platform does not support atomic, it depends on how the critical_section crates implements it. In single core scenario, this is just about disabling the interrupt for the few instructions. On multi core device, there might be a spinlock. but how else could we implement it?

What happens if there are multiple cores/CPUs in play)? Or are we sure that that that's not a problem for these targets?

The atomic-polyfill README states: https://github.com/embassy-rs/atomic-polyfill

Note: polyfill is based on critical sections using the critical-section crate. The default implementation is based on disabling all interrupts, so it's unsound on multi-core targets. It is possible to supply a custom
critical section implementation, check the critical-section docs for details.

Could you bump a version in Cargo.toml as well, such that this gets a release automatically when merged?

Should it be 1.8.1 or 1.9 ?

@matklad
Copy link
Owner

matklad commented Dec 14, 2021

Should it be 1.8.1 or 1.9 ?

1.9

so it's unsound on multi-core targets

Yeah, it is good to know. Let's add some scary-sounding warning to the feature in Cargo.toml, something like "this can be unsound. Please read docs of atomic-polyfill (link) and make sure you understand all the implications".

@matklad
Copy link
Owner

matklad commented Dec 14, 2021

I guess, my initial knee-jerk reaction was overly negative. I would push back strongly against this being enabled by default, but having sketchy things under a feature flag is totally fine :-)

@matklad
Copy link
Owner

matklad commented Dec 14, 2021

bors r+

Thanks!

@bors
Copy link
Contributor

bors bot commented Dec 14, 2021

Build succeeded:

@bors bors bot merged commit 44852cc into matklad:master Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

once_cell::race does not compile on ARM
2 participants