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

Implement pool for any 32/64-bit architecture that supports the corresponding atomics. #458

Merged
merged 4 commits into from
Feb 28, 2024

Conversation

reitermarkus
Copy link
Member

Closes #421.

@reitermarkus reitermarkus force-pushed the treiber-64 branch 3 times, most recently from e0f93ba to 0bbdb26 Compare February 19, 2024 14:18
@reitermarkus reitermarkus requested a review from a team February 19, 2024 14:18
@Dirbaio
Copy link
Member

Dirbaio commented Feb 25, 2024

do we know why the current code is impl'd only for x86? It could be because the treiber stack is only sound in x86 thanks to its stronger ordering guarantees. Or it could be because cfg(target_has_atomic) didn't exist back then and the original author thought "meh x86 is the only major 32bit arch with 64bit atomics so let's just hardcode it".

Would be good to know before we lift the restriction. I'm not familiar at all with these fancier atomics stuff.

@jamesmunns
Copy link
Member

do we know why the current code is impl'd only for x86? It could be because the treiber stack is only sound in x86 thanks to its stronger ordering guarantees. Or it could be because cfg(target_has_atomic) didn't exist back then and the original author thought "meh x86 is the only major 32bit arch with 64bit atomics so let's just hardcode it".

Would be good to know before we lift the restriction. I'm not familiar at all with these fancier atomics stuff.

I think the trick here is that the trieber stack is susceptible to the ABA problem: https://en.wikipedia.org/wiki/Treiber_stack#Correctness

On platforms with TRUE LL/SC atomics (e.g. Arm), this can be implemented correctly (tho might require custom asm? Unsure the state of things today, but see #180 and changes made in #315).

However it looks like this trick (again, I'm out of the loop) is to allow this in cases where the platform supports atomics that are LARGER than pointer widths, so you can CAS a pointer AND a unique tag to avoid the ABA problem, or at least make it sufficiently rare.

Dirbaio
Dirbaio previously approved these changes Feb 27, 2024
Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

reading more about it, I think i'm somewhat comfortable with the x64 impl being sound if it uses AtomicU128. Shame it's nightly-only but it's better than nothing :)

@reitermarkus
Copy link
Member Author

I think the trick here is that the trieber stack is susceptible to the ABA problem: > https://en.wikipedia.org/wiki/Treiber_stack#Correctness

Thanks for the link, I understand now what the problem is and how using a value twice the size of the pointer width fixes it.

I have added an example explaining how it works, which is hopefully easy to follow.

I guess technically it's still possible to be incorrect if one threads pops/pushes the top of the stack usize::MAX times between another's load and cas, but that may be practically impossible.

do we know why the current code is impl'd only for x86?

I would expect because AtomicI128 is still unstable, i.e. this only works on nightly or when using portable-atomic.

Dirbaio
Dirbaio previously approved these changes Feb 27, 2024
@Dirbaio Dirbaio added this pull request to the merge queue Feb 28, 2024
Merged via the queue into rust-embedded:main with commit 6cd26cc Feb 28, 2024
22 checks passed
@reitermarkus reitermarkus deleted the treiber-64 branch February 29, 2024 12:35
This pull request was closed.
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.

heapless::pool support for x86_64
3 participants