From 7c2496f4f1c0b35e254372022f17bde8952e5347 Mon Sep 17 00:00:00 2001 From: Yibo Cai Date: Fri, 7 Apr 2023 18:21:08 +0800 Subject: [PATCH] Fix RCU memory order bug on Arm Unit test "my_rcu_lock_test.multi_threaded_run" suffers from sporadic segfault on an 80 core Arm64 server. It's due to memory ordering bug in RCU implementation. Current code can be simplied as below: writer thread ------------- oldT = rcu_global_.exchange(newT, std::memory_order_release); // set new RCU while (rcu_readers_.load(std::memory_order_relaxed) > 0) // wait for all readers ; delete oldT; // delete old RCU reader thread ------------- rcu_readers_.fetch_add(1, std::memory_order_relaxed); // get lock visit(rcu_global_); // might visit deleted RCU !!!!!! rcu_readers_.fetch_sub(1, std::memory_order_relaxed); // release lock The problem is there's no memory ordering guarantee at both the writer and reader side using std::memory_order_relaxed. The reader may visit a deleted RCU and crash. We should use the default sequential consistency memory order. There's often no solid performance benefits to use relaxed memory order, but the risks are very high and may lead to hidden bugs hard to find. Change-Id: I4679205fcd04a04540a9cad5875f02cfda8179ca --- include/my_rcu_lock.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/include/my_rcu_lock.h b/include/my_rcu_lock.h index 1ff5910e..96762bbe 100644 --- a/include/my_rcu_lock.h +++ b/include/my_rcu_lock.h @@ -131,8 +131,8 @@ class MyRcuLock { @return the copy of the global */ const T *rcu_read() { - rcu_readers_.fetch_add(1, std::memory_order_relaxed); - return rcu_global_.load(std::memory_order_relaxed); + rcu_readers_.fetch_add(1); + return rcu_global_.load(); } /** @@ -148,7 +148,7 @@ class MyRcuLock { MyRcuLock::read() should not be used after calling @ref MyRcuLock::rcu_end_read() */ - void rcu_end_read() { rcu_readers_.fetch_sub(1, std::memory_order_relaxed); } + void rcu_end_read() { rcu_readers_.fetch_sub(1); } /** Low level API: write a new global and return the old one @@ -168,7 +168,7 @@ class MyRcuLock { MY_COMPILER_DIAGNOSTIC_POP() const T *rcu_write(const T *newT) { - return rcu_global_.exchange(newT, std::memory_order_release); + return rcu_global_.exchange(newT); } /** @@ -184,7 +184,7 @@ class MyRcuLock { */ bool wait_for_no_readers() { bool stopped = false; - while (rcu_readers_.load(std::memory_order_relaxed) > 0) + while (rcu_readers_.load() > 0) ; return stopped; } -- 2.25.1