5:35 p.m.
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(a)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(a)google.com>