spacejam / sled

the champagne of beta embedded databases
Apache License 2.0
7.89k stars 377 forks source link

Modify issues related to atomic::ordering usage #1467

Open wang384670111 opened 10 months ago

wang384670111 commented 10 months ago

https://github.com/spacejam/sled/blob/005c023ca94d424d8e630125e4c21320ed160031/src/metrics.rs#L131-L132 https://github.com/spacejam/sled/blob/005c023ca94d424d8e630125e4c21320ed160031/src/metrics.rs#L137-L138 https://github.com/spacejam/sled/blob/005c023ca94d424d8e630125e4c21320ed160031/src/metrics.rs#L140-L141 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.

https://github.com/spacejam/sled/blob/005c023ca94d424d8e630125e4c21320ed160031/src/pagecache/heap.rs#L262 https://github.com/spacejam/sled/blob/005c023ca94d424d8e630125e4c21320ed160031/src/histogram.rs#L45-L47 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 https://github.com/spacejam/sled/blob/005c023ca94d424d8e630125e4c21320ed160031/src/ebr/list.rs#L135-L154 https://github.com/spacejam/sled/blob/005c023ca94d424d8e630125e4c21320ed160031/src/ebr/list.rs#L184 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.

https://github.com/spacejam/sled/blob/005c023ca94d424d8e630125e4c21320ed160031/src/pagecache/pagetable.rs#L86 https://github.com/spacejam/sled/blob/005c023ca94d424d8e630125e4c21320ed160031/src/pagecache/pagetable.rs#L124-L127 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. https://github.com/spacejam/sled/blob/005c023ca94d424d8e630125e4c21320ed160031/src/tree.rs#L112 Here root synchronizes with the context field, so using Acquire/Release can ensure the correctness of the program. https://github.com/spacejam/sled/blob/005c023ca94d424d8e630125e4c21320ed160031/src/threadpool.rs#L97-L98 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.

https://github.com/spacejam/sled/blob/005c023ca94d424d8e630125e4c21320ed160031/src/lazy.rs#L91 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. https://github.com/spacejam/sled/blob/005c023ca94d424d8e630125e4c21320ed160031/src/lazy.rs#L63-L84 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.