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

Support architectures without atomics with polyfill #115

Merged
merged 7 commits into from
Apr 25, 2023

Conversation

ia0
Copy link
Contributor

@ia0 ia0 commented Apr 12, 2023

No description provided.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
Could you add some documentation about the need to add a critical-section implementation when using this on the corresponding architectures?
Could you also add an entry about this change to the changelog?
Actually, atomic-polyfill should be replaced with portable-atomic but it is not there yet so I'm fine with going ahead with this merge and replacing it later on.
See rust-embedded/heapless#328 for context.

@ia0
Copy link
Contributor Author

ia0 commented Apr 24, 2023

Actually, atomic-polyfill should be replaced with portable-atomic but it is not there yet so I'm fine with going ahead with this merge and replacing it later on.

Oh I wasn't aware of that, good to know. Should I create an issue and/or add a # TODO(#118): Use portable-atomic. in the Cargo.toml to track this?

@eldruin
Copy link
Member

eldruin commented Apr 24, 2023

Yes, please create a follow-up issue.

@ia0
Copy link
Contributor Author

ia0 commented Apr 24, 2023

Yes, please create a follow-up issue.

I created #118. Should I also add a TODO in the Cargo.toml?

Also, I might be misunderstanding something, but it looks like taiki-e/portable-atomic#51 is merged. And I would assume we can already use portable-atomic. In which case, it's probably better to do it directly in this PR no?

@Dirbaio
Copy link

Dirbaio commented Apr 24, 2023

I intend to deprecate the atomic-polyfill crate, please don't add it as a dependency.

Which architecture are you trying to use? Is it riscv32imc? It is capable of atomic loads/stores, It is a Rust bug that it is not currently exposed. See embassy-rs/embassy#745 (comment), rust-lang/rust#99668 . This crate only does atomic loads/stores, it doesn't use CAS operations, so it should be able to use the native core::sync::atomic once that Rust issue is fixed. Also, atomic-polyfill incurs the overhead of acquiring/releasing a critical section for loads/stores, because it's designed to also support CAS. The native atomics won't have this overhead.

(btw embassy-usb already works on architectures with this rust bug, as an alternative to usb-device, since it doesn't use atomics at all).

The trouble with portable-atomic is it requires nonstandard RUSTFLAGS=--cfg=blah to build for single core chips (or you get unnecessary overhead of critical sections for loads/stores). I've proposed changing it to Cargo features here, but haven't heard back. taiki-e/portable-atomic#94

@taiki-e
Copy link

taiki-e commented Apr 24, 2023

FWIW, if the required atomic operations are only load/store, portable-atomic always provides them on RISC-V, even without critical-section features or single-core cfg.

@ia0
Copy link
Contributor Author

ia0 commented Apr 24, 2023

Thanks for the explanation and pointers!

Indeed, I confirm using portable-atomic works out of the box and doesn't need the --cfg for CAS operations. I also disabled the default feature as suggested by the documentation since usb-device doesn't use atomic types larger than the pointer width. Note that I've also reverted the documentation change since there's nothing to be done by the end-user anymore.

If this PR looks good, I'll just close #118 as obsolete.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you everybody!

@eldruin eldruin merged commit 028abb9 into rust-embedded-community:master Apr 25, 2023
3 checks passed
@ia0 ia0 deleted the polyfill branch April 25, 2023 08:04
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.

None yet

4 participants