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

Revert "Fix fence on non-x86 arch and miri (#16)" #18

Merged
merged 3 commits into from
Jul 20, 2022
Merged

Revert "Fix fence on non-x86 arch and miri (#16)" #18

merged 3 commits into from
Jul 20, 2022

Conversation

taiki-e
Copy link
Collaborator

@taiki-e taiki-e commented Jul 20, 2022

Based on the discussion in #17:

  • Revert Fix fence on non-x86 arch and miri #16 for now (v1.2.3 is already yanked)
  • Use compiler_fence in full_fence on x86
  • Do not use x86 specific fence on Miri (because Miri doesn't understand it)

The fence for the unbounded queue needs to be fixed again in some way, but for now, I'm going to revert all of them.

@sbarral Any thoughts?

@sbarral
Copy link

sbarral commented Jul 20, 2022

@sbarral Any thoughts?

Nope, looks good to me :-)

@taiki-e taiki-e merged commit d61005f into master Jul 20, 2022
@taiki-e taiki-e deleted the fence branch July 20, 2022 14:18
@RalfJung
Copy link

This is still doing the fence before the load though, how does that make sense?

@sbarral
Copy link

sbarral commented Jul 26, 2022

This is still doing the fence before the load though, how does that make sense?

I do not know the full context of your reply (I understand this has been discussed someplace else) so sorry if my comment is irrelevant, but in general there certainly are legit use cases for SeqCst fences before a load. For instance:

// Thread A:
x.store(true, Ordering::relaxed);
fence(Ordering::SeqCst);
let y_val = y.load(Ordering::relaxed);

// Thread B:
y.store(true, Ordering::relaxed);
fence(Ordering::SeqCst);
let x_val = x.load(Ordering::relaxed);

will ensure that either the x store will be visible in thread B or the y store will be visible in thread A, or both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants