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

Use atomics using atomic-polyfill #1240

Closed
wants to merge 1 commit into from

Conversation

psykora
Copy link

@psykora psykora commented Feb 26, 2023

Replace use of core::sync::atomic by atomic_polyfill

Motivation: When playing with rust and esp-rs/esp-wifi, I noticed that the examples does not compile with the embassy master.
This is quick fix for the problem. I noticed that the polyfill is used as drop-in replacement on other places

  • Target: riscv32imc-unknown-none-elf
  • Device: esp32c3

@Dirbaio
Copy link
Member

Dirbaio commented Feb 26, 2023

See discussion here #745 (comment)

and rust-lang/rust#99668

@Dirbaio Dirbaio closed this Feb 26, 2023
@psykora
Copy link
Author

psykora commented Feb 26, 2023

Hi Dirbaio,

Thanks for pointing out there is another workaround. I didn't know it. (To be honest my experience with rust on MCU is close to zero 😳).

However, I still don't understand why we can't use atomic-polyfill here. What is different from other cases?

Rationale:

  • It is not newly introduced to embassy.
    It is used on numerous places (just quickly looked for occurrences of atomic_polyfill in this repository) for example.:
    embassy-executor/src/raw/run_queue.rs
  • To my understanding atomic-polyfill should just reexport core::sync::atomic:: on targets where there is native support.

Thanks for understanding,

@Dirbaio
Copy link
Member

Dirbaio commented Feb 26, 2023

It has overhead on targets without hardware CAS support, because it has to acquire a critical section for each operation. This is also needed in load/store operations to prevent them from racing with CAS operations. If you never do CAS on that atomic value, it's useless overhead.

That code you linked does do CAS operations.

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.

2 participants