Skip to content

Commit 6bc44c2

Browse files
daverigbychiyoung
authored andcommitted
[BP] MB-15292: Make CouchbaseAtomic::store() atomic
As identified by ThreadSantizer: WARNING: ThreadSanitizer: data race (pid=59833) Write of size 8 at 0x7d240000d3f0 by thread T8: #0 CouchbaseAtomic<unsigned long>::store(unsigned long const&, memory_order) /root/couchbase-3.0/ep-engine/src/atomic.h:84 (ep.so+0x0000000414ef) #1 CouchbaseAtomic<unsigned long>::operator=(unsigned long const&) /root/couchbase-3.0/ep-engine/src/atomic.h:105 (ep.so+0x0000000401f5) #2 Warmup::scheduleEstimateDatabaseItemCount() /root/couchbase-3.0/ep-engine/src/warmup.cc:500 (ep.so+0x000000277991) #3 Warmup::step() /root/couchbase-3.0/ep-engine/src/warmup.cc:816 (ep.so+0x000000275124) #4 Warmup::transition(int, bool) /root/couchbase-3.0/ep-engine/src/warmup.cc:853 (ep.so+0x0000002754ff) #5 Warmup::createVBuckets(unsigned short) /root/couchbase-3.0/ep-engine/src/warmup.cc:491 (ep.so+0x00000027785f) #6 WarmupCreateVBuckets::run() /root/couchbase-3.0/ep-engine/src/warmup.h:234 (ep.so+0x00000028cde9) #7 ExecutorThread::run() /root/couchbase-3.0/ep-engine/src/executorthread.cc:110 (ep.so+0x0000001a2581) #8 launch_executor_thread(void*) /root/couchbase-3.0/ep-engine/src/executorthread.cc:34 (ep.so+0x0000001a1a5a) #9 platform_thread_wrap /root/couchbase-3.0/platform/src/cb_pthreads.c:19 (libplatform.so.0.1.0+0x000000002f14) Previous atomic write of size 8 at 0x7d240000d3f0 by main thread (mutexes: write M670470284968504712): #0 __tsan_atomic64_fetch_add <null>:0 (engine_testapp+0x00000008cb48) #1 CouchbaseAtomic<unsigned long>::load(memory_order) const /root/couchbase-3.0/ep-engine/src/atomic.h:77 (ep.so+0x0000000446b4) #2 CouchbaseAtomic<unsigned long>::operator unsigned long() const /root/couchbase-3.0/ep-engine/src/atomic.h:101 (ep.so+0x000000044575) #3 Warmup::addStats(void (*)(char const*, unsigned short, char const*, unsigned int, void const*), void const*) const /root/couchbase-3.0/ep-engine/src/warmup.cc:901 (ep.so+0x00000027d4ea) #4 EventuallyPersistentEngine::getStats(void const*, char const*, int, void (*)(char const*, unsigned short, char const*, unsigned int, void const*)) /root/couchbase-3.0/ep-engine/src/ep_engine.cc:4405 (ep.so+0x0000001155a9) #5 EvpGetStats(engine_interface*, void const*, char const*, int, void (*)(char const*, unsigned short, char const*, unsigned int, void const*)) /root/couchbase-3.0/ep-engine/src/ep_engine.cc:214 (ep.so+0x0000000fa102) #6 mock_get_stats /root/couchbase-3.0/memcached/programs/engine_testapp/engine_testapp.c:194 (engine_testapp+0x0000000aeecd) #7 wait_for_warmup_complete(engine_interface*, engine_interface_v1*) /root/couchbase-3.0/ep-engine/tests/ep_test_apis.cc:864 (ep_perfsuite.so+0x00000001b1cb) #8 test_setup(engine_interface*, engine_interface_v1*) /root/couchbase-3.0/ep-engine/tests/ep_testsuite_common.cc:128 (ep_perfsuite.so+0x00000000dc03) #9 execute_test /root/couchbase-3.0/memcached/programs/engine_testapp/engine_testapp.c:1037 (engine_testapp+0x0000000ab85a) #10 main /root/couchbase-3.0/memcached/programs/engine_testapp/engine_testapp.c:1296 (engine_testapp+0x0000000a9a19) Change-Id: I260942712ca471c0d2e0fa3ebc4793d694f9b237 Reviewed-on: http://review.couchbase.org/51973 Tested-by: buildbot <[email protected]> Reviewed-by: Jim Walker <[email protected]> Reviewed-by: Chiyoung Seo <[email protected]> Reviewed-on: http://review.couchbase.org/53502 Tested-by: Chiyoung Seo <[email protected]>
1 parent 875d7a4 commit 6bc44c2

File tree

2 files changed

+13
-2
lines changed

2 files changed

+13
-2
lines changed

src/atomic.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,7 @@ class CouchbaseAtomic {
8181

8282
void store(const T &newValue, memory_order sync = memory_order_acq_rel) {
8383
(void) sync;
84-
value = newValue;
85-
ep_sync_synchronize();
84+
ep_sync_lock_test_and_set(&value, newValue);
8685
}
8786

8887

tests/module_tests/atomic_test.cc

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,17 @@ static void testAtomicInt() {
6262
cb_assert(intgen.latest() == (numThreads * numIterations));
6363
}
6464

65+
static void testAtomicUint64() {
66+
// Check that we can correctly load / store from values larger than 32bits.
67+
AtomicValue<uint64_t> value;
68+
uint64_t expected_val = 1ul << 33;
69+
value.store(expected_val);
70+
cb_assert(value == expected_val);
71+
72+
value.store(0);
73+
cb_assert(value == 0);
74+
}
75+
6576
static void testSetIfLess() {
6677
AtomicValue<int> x;
6778

@@ -127,6 +138,7 @@ int testAtomicCompareExchangeStrong(void) {
127138
int main() {
128139
alarm(60);
129140
testAtomicInt();
141+
testAtomicUint64();
130142
testSetIfLess();
131143
testSetIfBigger();
132144
testAtomicDouble();

0 commit comments

Comments
 (0)