Kyle Lippincott has uploaded this change for review. ( https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/1199063?usp=email ) Change subject: atomics: remove broken __sync-based implementations ...................................................................... atomics: remove broken __sync-based implementations The "new" version of these functions, using functions with an `__atomic` prefix have been available since gcc 4.7, released March 2012, which is more than 12 years ago. We can just assume that the compilers in use today support them; if this is incorrect, then we'll get an error message during compilation. This is not purely a code cleanup. Clang presents itself as gcc 4.2, which means it's using the `__sync` versions of the functions per the version checking being done at the top of the file, even though it supports the `__atomic` versions. This is a missed performance opportunity, even if the function implementations were correct, as the `__atomic` versions may be faster. There's a reason we prefer them if they're available, after all. Furthermore, the versions of these functions that are based on the `__sync` versions are broken. An example: ``` static inline int32_t atomic_inc_unless_0_int32_t(int32_t *var) { int32_t cur, newv; bool changed; cur = atomic_fetch_int32_t(var); do { if (cur == 0) return 0; newv = cur + 1; changed = __sync_val_compare_and_swap(var, cur, newv) != cur; } while (!changed); return newv; } ``` This has multiple issues: - In the 99.99% case, we'll: - fetch `*var` into `cur`, - set `newv = cur + 1`, - then atomically (possibly "transactionally") perform an operation equivalent to: ``` old_value = *var; if (*var == cur) *var = newv; return old_value; ``` - This then does `changed = old_value != cur`, which is incorrect since `__sync_val_compare_and_swap` returns the _original_ value (whether it changed or not). This isn't correct; this should be `changed = old_value == cur`, not `!=`. - Basically: if nothing weird happens, we always go through the loop _twice_. - In the tragic 0.01% case, where some other thread modifies `var` between the `atomic_fetch_int32_t` and `__sync_val_compare_and_swap`, we'll get that other thread's value from `__sync_val_compare_and_swap`, which won't be equal to `cur`, so we just skip incrementing at all. - The implementation can't be fixed by just using the correct comparison, because then you run into the possibility of an infinite loop: ``` cur = 10; // atomic_fetch_int32_t(var) do { newv = 11; // cur + 1; // !!!! Some other thread sets *var to 50 !!!! changed = false; // 50 == 10, since sync_val... returns 50 } while (!changed); // loops until some other thread sets it to 10 ``` - We could capture the value from `__sync_val_compare_and_swap`, and update `cur`, or we could switch to `__sync_bool_compare_and_swap` and move the `atomic_fetch_int32_t` into the `do {} while` loop to resolve this other issue. - It seems like there might have been some confusion about what the `_and_swap` part of the function name meant (i.e. that it updated `cur`), and also an assumption that the function would return the _new_ value, like `__atomic_add_fetch` does. Ultimately, however, these `__sync` functions are old, the replacements (`__atomic`) are also old and broadly available, and maintaining two implementations of these functions is a cost that we shouldn't have to take. Remove the broken, legacy versions of the atomic functions that use the `__sync` compiler builtins. Change-Id: I1851d4e37cb9ecfeb5959736f682a9625cc42be5 Signed-off-by: Kyle Lippincott <spectral@google.com> --- M src/include/abstract_atomic.h M src/include/atomic_utils.h 2 files changed, 0 insertions(+), 684 deletions(-) git pull ssh://review.gerrithub.io:29418/ffilz/nfs-ganesha refs/changes/63/1199063/1 -- To view, visit https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/1199063?usp=email To unsubscribe, or for help writing mail filters, visit https://review.gerrithub.io/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: ffilz/nfs-ganesha Gerrit-Branch: next Gerrit-Change-Id: I1851d4e37cb9ecfeb5959736f682a9625cc42be5 Gerrit-Change-Number: 1199063 Gerrit-PatchSet: 1 Gerrit-Owner: Kyle Lippincott <spectral@google.com>