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

Using Release in the store operation for make_mut just prevent out-of-thin-air value? #133284

Open
xmh0511 opened this issue Nov 21, 2024 · 0 comments
Labels
A-atomic Area: Atomics, barriers, and sync primitives C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@xmh0511
Copy link

xmh0511 commented Nov 21, 2024

In https://doc.rust-lang.org/src/alloc/sync.rs.html#2267, the make_mut is implemented as

if this.inner().strong.compare_exchange(1, 0, Acquire, Relaxed).is_err(){
  // ...
}else if this.inner().weak.load(Relaxed) != 1 {  // #0
 //...
}else{
  this.inner().strong.store(1, Release);  // #1
}

while upgrade is

if self.inner()?.strong.fetch_update(Acquire, Relaxed, checked_increment).is_ok(){
   Some(...)
}else{
 None
}

The Release at #1 concerns this case

// thread 1:
let mut_ref = Arc::make_mut(&mut my_arc);
*mut_ref = 10;

//thread 2: 
let arc = weak.upgrade().unwrap();
drop(weak);
println!("{}", *arc);

In this case, upgrade will return Some if it reads #1 and #1 is executed only #0 reads the value written by weak.fetch_sub(1, Release) in drop(weak);, so the model can be simplified as

weak = 2;
strong = 0;
// thread 1:
if weak.load(Relaxed) == 1{  // #1
   strong.store(1, Relaxed) // if the Release is changed to Relaxed  // #2
}
// thread 2:
while strong.load(Relaxed) !=0{ // #3
}
weak.store(1, Relaxed); // #4

#1 read #4, #4 is executed only if #3 read #2, #2 is executed only if #1 read #4. It depends on the out-of-thin-air value. Do we need a release memory order here to prevent OOTD?

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 21, 2024
@jieyouxu jieyouxu added T-libs Relevant to the library team, which will review and decide on the PR/issue. C-discussion Category: Discussion or questions that doesn't represent real issues. A-atomic Area: Atomics, barriers, and sync primitives and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-atomic Area: Atomics, barriers, and sync primitives C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants