This patch fixes a subtle yet critical bug in the implementation of the
atomic_fetchadd_* functions used in the bsd subtree of OSv source code.
This bug is a root cause of the issues #1189 and #1190 and affects stability
of ZFS and networking stack on aarch64.
The atomic_fetchadd_*() are implemented in inlined assembly and provide the
functionality to atomically add/subtract to/from a 4- or 8-bytes long value
in memory and return old value of it before update. The assembly made of
four instructions in essence implements simple loop to read a value from memory,
add a specified delta, update memory with new value and finally check if the
update was successful. The pre-patch version of this code is almost correct
and works properly if the atomic update is successful in the 1st attempt.
However it works incorrectly if that update fails and it needs to retry
which is quite rare.
As an example the generate machine code might look like this:
0x0000100000119690 <+0>: ldaxr x2, [x0]
0x0000100000119694 <+4>: add x1, x1, x2
0x0000100000119698 <+8>: stlxr w3, x1, [x0]
0x000010000011969c <+12>: cbnz w3, 0x100000119690
<atomic_add_64(uint64_t volatile*, int64_t)>
One can eventually notice that the x1 register holding a result (sum) to be
updated to memory is re-used across iterations and would behave like an
accumulator which is wrong. We have to fix the inline assembly to make
sure that separate register is used for that. Rather than trying to fix
existing code, this patch updates both atomic_fetchadd_*() functions
with the copies of atomic_fetchadd_32 and atomic_fetchadd_64 from current
enough version of FreeBSD code -
sys/arm64/include/atomic.h@119a353e3d9d45650e109600160caca173ac8a53
and tweaked to match the types of val, tmp and res variables.
Please note the FreeBSD version of the code uses ldxr/stxr instructions
instead of ldaxr/stlxr ones with require/release memory ordering semantics
which are excessive for atomic_fetchadd_*().
Fixes #1189
Fixes #1190
Signed-off-by: Waldemar Kozaczuk <jwkozac...@gmail.com>
---
bsd/aarch64/machine/atomic.h | 46 ++++++++++++++++++++++--------------
1 file changed, 28 insertions(+), 18 deletions(-)
diff --git a/bsd/aarch64/machine/atomic.h b/bsd/aarch64/machine/atomic.h
index eb447d8e..55532585 100644
--- a/bsd/aarch64/machine/atomic.h
+++ b/bsd/aarch64/machine/atomic.h
@@ -83,28 +83,38 @@ int atomic_cmpset_long(volatile u_long *dst, u_long expect,
u_long src);
static __inline u_int atomic_fetchadd_int(volatile u_int *p, u_int val)
{
- u_int result;
- u_int status;
- __asm __volatile("1: ldaxr %w0, %1 ; "
- " add %w2, %w2, %w0 ; "
- " stlxr %w3, %w2, %1 ; "
- " cbnz %w3, 1b ; "
- : "=&r"(result), "+Q"(*p), "+r"(val), "=&r"(status));
-
- return result;
+ u_int tmp, ret;
+ u_int res;
+
+ __asm __volatile(
+ "1: ldxr %w2, [%3] \n"
+ " add %w0, %w2, %w4 \n"
+ " stxr %w1, %w0, [%3] \n"
+ " cbnz %w1, 1b \n"
+ : "=&r"(tmp), "=&r"(res), "=&r"(ret)
+ : "r" (p), "r" (val)
+ : "memory"
+ );
+
+ return ret;
}
static __inline u_long atomic_fetchadd_long(volatile u_long *p, u_long val)
{
- u_long result;
- u_int status;
- __asm __volatile("1: ldaxr %0, %1 ; "
- " add %2, %2, %0 ; "
- " stlxr %w3, %2, %1 ; "
- " cbnz %w3, 1b ; "
- : "=&r"(result), "+Q"(*p), "+r"(val), "=&r"(status));
-
- return result;
+ u_long tmp, ret;
+ u_int res;
+
+ __asm __volatile(
+ "1: ldxr %2, [%3] \n"
+ " add %0, %2, %4 \n"
+ " stxr %w1, %0, [%3] \n"
+ " cbnz %w1, 1b \n"
+ : "=&r"(tmp), "=&r"(res), "=&r"(ret)
+ : "r" (p), "r" (val)
+ : "memory"
+ );
+
+ return ret;
}
static __inline void atomic_store_rel_int(volatile u_int *p, u_int val)
--
2.27.0
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/20220420181404.53392-1-jwkozaczuk%40gmail.com.
