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.

Sign up for GitHub

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

Jump to bottom

Is MsQueue missing memory barriers? #12

Closed
sorear opened this issue Oct 18, 2015 · 1 comment
Closed

Is MsQueue missing memory barriers? #12

sorear opened this issue Oct 18, 2015 · 1 comment

Comments

@sorear
Copy link

sorear commented Oct 18, 2015

I'm looking over the MsQueue code to practice reasoning about memory barriers, and I see a couple things that appear odd:

  • I don't think any of these memory orderings are required to be effective. Per the LLVM spec, Acquire only does something when paired with Release and conversely, but MsQueue only uses Release and Relaxed on the next pointers and Acquire and Relaxed on the head and tail pointers. So I think the implementation is allowed to replace all of the orderings with Relaxed. ?
  • If we ignore that and pretend Acquire can synchronize with Relaxed, pop() is still doing a Relaxed fetch of head.next and then reading from the next record's data without additional memory barriers, which could get uninitialized memory on Alpha ? (It looks like this is the kind of thing memory_order_consume is for, as this is technically stronger than Relaxed but still requires no barriers on common hardware. Wonder when LLVM/Rust will get it.)
@aturon aturon closed this as completed in bbb8966 Nov 2, 2015
@aturon
Copy link
Collaborator

aturon commented Nov 2, 2015

You're correct. I've fixed the orderings -- thanks!

schets pushed a commit to schets/crossbeam that referenced this issue Jan 20, 2016
@aturon
Fix memory ordering on MsQueue
e9da311 
The `MsQueue` implementation incorrectly used `Relaxed` operations in a
few places, which resulted in a lack of pairing between `Acquire` and
`Release` operations.

Closes crossbeam-rs#12
exrook pushed a commit to exrook/crossbeam that referenced this issue Oct 7, 2020
@luser
Use serde for serializing stats instead of the ad-hoc CacheStats we w…
8f16f2c 
…ere using. Also fixes crossbeam-rs#12.

This change removes all the `CacheStats` / `CacheStatistics` types and adds a
`ServerInfo` struct which contains the existing `ServerStats` struct as a
field as well as the cache location and size info we're currently sending
in the stats response. We now just send a `ServerInfo` directly in the
server response using serde for serialization. The existing `print_stats`
code is moved into `ServerStats::print` and `ServerInfo::print` methods.

This lays the foundation to make it easy to implement JSON stats output.
@nical nical mentioned this issue Jan 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@sorear @aturon