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

Modify issues related to atomic::ordering usage #1467

Open
wang384670111 opened this issue Sep 7, 2023 · 0 comments
Open

Modify issues related to atomic::ordering usage #1467

wang384670111 opened this issue Sep 7, 2023 · 0 comments

Comments

@wang384670111
Copy link

sled/src/metrics.rs

Lines 131 to 132 in 005c023

pub tree_child_split_attempt: CachePadded<AtomicUsize>,
pub tree_child_split_success: CachePadded<AtomicUsize>,

sled/src/metrics.rs

Lines 137 to 138 in 005c023

pub tree_parent_split_attempt: CachePadded<AtomicUsize>,
pub tree_parent_split_success: CachePadded<AtomicUsize>,

sled/src/metrics.rs

Lines 140 to 141 in 005c023

pub tree_root_split_attempt: CachePadded<AtomicUsize>,
pub tree_root_split_success: CachePadded<AtomicUsize>,

These six atomic varieties don't synchronize with any other varieties, so just use Relaxed here is enough.
I think programmers have some misconceptions about Acquire, here fetch_add can manipulate the latest values, programmers may mistakenly think that when load uses "Acquire", it is guaranteed to see the modification operations (fetch_add) of other threads on atomic variables immediately, which is a misconception: with weaker memory models such as Relaxed, modifications to atomic variables by other threads are not immediately visible. In fact, Any memory order does not force all modifications to atomic variables to be immediately visible in other threads. Ordering only defines which ordering certain things happen(It can also be understood as addtional synchronization). Atomic types has a modification order, and each thread will see a consistent modification order, which means that if a modification of an atomic variable is not visible by another thread's RMW operation, all threads must agree to the modification order of the atomic variable. Relaxed guarantees a monotonic/consistent order when looking at just a single atomic variable. Therefore, the use of Relaxed here can ensure the correctness of the program.

tip: AtomicU32,

sled/src/histogram.rs

Lines 45 to 47 in 005c023

vals: Vec<AtomicUsize>,
sum: AtomicUsize,
count: AtomicUsize,

The same problem arises with the ordering use of tip in heao.rs and the use of vals, sum, and count in histogram.rs, neither of which synchronizes other variables, so Relaxed can be used for the operation of these atomic variables

sled/src/ebr/list.rs

Lines 135 to 154 in 005c023

let to = &self.head;
// Get the intrusively stored Entry of the new element to insert.
let entry: &Entry = C::entry_of(container.deref());
// Make a Shared ptr to that Entry.
#[allow(trivial_casts)]
let entry_ptr = Shared::from(entry as *const _);
// Read the current successor of where we want to insert.
let mut next = to.load(Relaxed, guard);
loop {
// Set the Entry of the to-be-inserted element to point to the previous successor of
// `to`.
entry.next.store(next, Relaxed);
match to.compare_and_set_weak(next, entry_ptr, Release, guard) {
Ok(_) => break,
// We lost the race or weak CAS failed spuriously. Update the successor and try
// again.
Err(err) => next = err.current,
}
}

let mut curr = self.head.load(Relaxed, &guard);

Line 184 uses the wrong ordering, where head is a pointer to entry, and the data written to the entry must be visible when line 184 executes head.load(This can be understood as the release-consume case in C++, but Rust removes the use of consume), Therefore, the load here should use Acquire to ensure the correctness of the program, If you use Relaxed, there is a chance (although, a small one) for the memory tonot be in a consistent state.

let shared_child = child.load(Relaxed, &guard);

let tip = self.traverse(pid, guard);
let shared = Owned::new(item).into_shared(guard);
let old = tip.swap(shared, Release, guard);

Line 86 uses the wrong ordering, child is a pointer to T, and when executing child.load, the content pointed to by the pointer must be visible. 'Release' memory ordering is employed when adding or modifying nodes. However, the load with Ordering::Relaxed doesn't establish a synchronization relationship. This scenario might result in a situation where, one thread has already called insert to modify nodes, but another thread calls the drop_iter method and invokes the 'drop' function, the node might not have been fully initialized yet, causing the pointer to the address to remain null(This can be understood as the release-consume case in C++, but Rust removes the use of consume).Therefore, the load here should use Acquire to ensure the correctness of the program, If you use Relaxed, there is a chance (although, a small one) for the memory tonot be in a consistent state.
pub(crate) root: AtomicU64,

Here root synchronizes with the context field, so using Acquire/Release can ensure the correctness of the program.

sled/src/threadpool.rs

Lines 97 to 98 in 005c023

temporary_threads: AtomicUsize,
spawning: AtomicBool,

Here temporary_threads does not synchronize other variables, it is just a global counter, you can use Relaxed.
The spawning here is only used in the perform_work function. There may be multiple threads executing at the same time, but spawning does not synchronize other variables. Spawn has consistent modification consistency for each thread, so I think store can With Relaxed, RMW can also use Relaxed.

let old = self.value.swap(value_ptr, SeqCst);

Here value is AtomicPtr, the methods load and store read and write the address. To load the contents is pretty much another operation, but it is not atomic and it is very unsafe, it is best to use Acquire/Release to ensure that the pointer to the content is visible. The swap of 91 rows of value can use AcqRel, here about the use of init_mu, I think that 94 rows because 91 rows use AcqRel, so the sorting limit has been made, so the operation of 94 rows cannot be sorted in front of the 91 row operation, so the ordering of init_mu can use Relaxed.

sled/src/lazy.rs

Lines 63 to 84 in 005c023

.init_mu
.compare_exchange(false, true, SeqCst, SeqCst)
.is_err()
{
// `hint::spin_loop` requires Rust 1.49.
#[allow(deprecated)]
std::sync::atomic::spin_loop_hint();
}
{
let value_ptr = self.value.load(Acquire);
// we need to check this again because
// maybe some other thread completed
// the initialization already.
if !value_ptr.is_null() {
let unlock = self.init_mu.swap(false, SeqCst);
assert!(unlock);
#[allow(unsafe_code)]
unsafe {
return &*value_ptr;
}
}

The init_mu of line 63 needs to synchronize the value of line 76. Acquire can be used for success in RMW, fail does not synchronize other variables, you can use Relaxed. The 78-line init_mu is just a multithreaded signal and does not synchronize other variables, you can use Relaxed.

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

No branches or pull requests

1 participant