From: Waldemar Kozaczuk <jwkozac...@gmail.com> Committer: Waldemar Kozaczuk <jwkozac...@gmail.com> Branch: master
aarch64: fix atomic_fetchadd_int and atomic_fetchadd_long 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> --- diff --git a/bsd/aarch64/machine/atomic.h b/bsd/aarch64/machine/atomic.h --- 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) -- 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/00000000000094314505de39b8f7%40google.com.