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 a mutex for 64 bit atomics on xtensa targets #4460

Closed
wants to merge 1 commit into from

Conversation

ellenhp
Copy link

@ellenhp ellenhp commented Feb 1, 2022

Motivation

I'm using a library that includes tokio as a transitive dependency on an xtensa architecture (ESP32). The ESP32 doesn't have 64 bit atomics, so tokio fails to build.

Solution

Adding xtensa to the list of target architectures that use mutexes to simulate atomic 64 bit integers. :)

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Feb 1, 2022
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thanks.

@ellenhp ellenhp marked this pull request as ready for review February 1, 2022 15:22
@ellenhp
Copy link
Author

ellenhp commented Feb 1, 2022

Spoke a bit with some Rust ESP-32 people and they indicated this was the right way to go about this change. Their only warning was that most of tokio still won't build which I knew already. Ready for review. Thank you!

@Darksonn
Copy link
Contributor

Darksonn commented Feb 2, 2022

I don't really understand why CI is stuck. Are you able to push an empty commit?

@ellenhp ellenhp force-pushed the no_64b_atomics_on_xtensa branch from 3227fae to c9b213d Compare February 4, 2022 05:13
@ellenhp
Copy link
Author

ellenhp commented Feb 4, 2022

I just amended the commit without changing anything and then force-pushed, hopefully that's fine. I know it says somewhere not to rebase and force-push during review but I assume that's just because it can make review hard for larger PRs. Hopefully for 6 lines of diff it was fine to do that.

@Darksonn
Copy link
Contributor

Darksonn commented Feb 4, 2022

Yes, it's ok in this case. Another thing you could have done is push an empty commit with git commit --allow-empty

@ellenhp ellenhp marked this pull request as draft February 6, 2022 22:13
@ellenhp
Copy link
Author

ellenhp commented Feb 6, 2022

I spoke a bit more with some Rust ESP32 people and we're considering maybe changing our max atomic width to 64 and implementing the 64 bit atomic types with a mutex. Tokio handles these platforms explicitly but much of the rust ecosystem assumes 64 atomics will always exist, so I suggested doing that as a potential option. I have no idea if that's the right thing to do or the wrong thing to do, but I wanted to put this into a draft state while people with more experience weigh in. Would hate to have to revert something if it got merged in the meantime.

@Darksonn
Copy link
Contributor

Darksonn commented Feb 7, 2022

Let me know what you find out.

@Darksonn
Copy link
Contributor

Darksonn commented Jun 1, 2022

Any status on this?

@ellenhp
Copy link
Author

ellenhp commented Jun 1, 2022

Hey! I'm not sure because I hit another blocker (Brian Smith choosing not to review briansmith/ring#1459) for my project so it ended up not really being possible to use the ESP32 for what I wanted. I haven't been in touch with the rest of the community since then.

@Darksonn
Copy link
Contributor

Darksonn commented Jun 1, 2022

Okay. I will close this for now. If anyone needs this in the future, they can open a new PR.

@Darksonn Darksonn closed this Jun 1, 2022
@MabezDev
Copy link

@ellenhp FYI we recently changed the STD targets to support 64bit atomics so in theory this PR is no longer needed anyway :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-sync Module: tokio/sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants