From 2c6e95c8e4397127ec4aa769fd60d1d20c418430 Mon Sep 17 00:00:00 2001 From: Dave Rigby Date: Wed, 13 Jul 2022 15:53:59 +0100 Subject: [PATCH] MB-47267: Make ObjectRegistry getAllocSize atomic As observed in tests in patch to fix MB-47267 ("MB-47267 / MB-52383: Make backfill during warmup a PauseResume task"), ObjectRegister getAllocSize can be read and written by different threads without synchronisation when EP engine instances are destroyed and re-created: WARNING: ThreadSanitizer: data race (pid=128791) Read of size 8 at 0x7f584d8d48c0 by thread T41 (mutexes: write M333120309177634496, write M279640201042175720): #0 ObjectRegistry::onCreateBlob(Blob const*) ../kv_engine/engines/ep/src/objectregistry.cc:85 (ep.so+0x0000002d60aa) #1 Blob::Blob(char const*, unsigned long) ../kv_engine/engines/ep/src/blob.cc:51 (ep.so+0x00000006ba08) #2 Blob::New(char const*, unsigned long) ../kv_engine/engines/ep/src/blob.cc:26 (ep.so+0x00000006ba56) #3 vbucket_transition_state::toItem(Item&) const ../kv_engine/engines/ep/src/vbucket_state.cc:31 (ep.so+0x0000002b1c39) #4 CheckpointManager::queueSetVBState(VBucket&) ../kv_engine/engines/ep/src/checkpoint_manager.cc:953 (ep.so+0x00000008030a) #5 Warmup::populateVBucketMap(unsigned short) ../kv_engine/engines/ep/src/warmup.cc:1508 (ep.so+0x0000002c55fd) #6 WarmupPopulateVBucketMap::run() ../kv_engine/engines/ep/src/warmup.cc:350 (ep.so+0x0000002d47dd) #7 ExecutorThread::run() ../kv_engine/engines/ep/src/executorthread.cc:190 (ep.so+0x0000001ec57b) #8 launch_executor_thread ../kv_engine/engines/ep/src/executorthread.cc:36 (ep.so+0x0000001ecb69) #9 CouchbaseThread::run() ../platform/src/cb_pthreads.cc:58 (libplatform_so.so.0.1.0+0x00000000a237) #10 platform_thread_wrap ../platform/src/cb_pthreads.cc:71 (libplatform_so.so.0.1.0+0x00000000a237) #11 (libtsan.so.0+0x00000002843b) Previous write of size 8 at 0x7f584d8d48c0 by main thread: #0 ObjectRegistry::initialize(unsigned long (*)(void const*)) ../kv_engine/engines/ep/src/objectregistry.cc:72 (ep.so+0x0000002d5ea7) #1 create_instance ../kv_engine/engines/ep/src/ep_engine.cc:1777 (ep.so+0x000000191c06) #2 create_engine_instance(engine_reference*, server_handle_v1_t* (*)(), EngineIface**) ../kv_engine/utilities/engine_loader.cc:95 (engine_testapp+0x0000004614b9) #3 MockTestHarness::create_bucket(bool, char const*) (engine_testapp+0x00000041f295) #4 test_reader_thread_starvation_warmup ../kv_engine/engines/ep/tests/ep_testsuite.cc:8246 (ep_testsuite.so+0x000000071909) #5 execute_test ../kv_engine/programs/engine_testapp/engine_testapp.cc:1402 (engine_testapp+0x00000041ac82) #6 main ../kv_engine/programs/engine_testapp/engine_testapp.cc:1675 (engine_testapp+0x00000041be5c) Location is global 'getAllocSize' of size 8 at 0x7f584d8d48c0 (ep.so+0x0000007708c0) In practice this race is most likely benign, as ObjectRegistry::initialize() is always passed the same argument to store to getAllocSize. However to silence TSan, change to an atomic variable accessed with memory_order_acquire / memory_order_release. Note this is the default ordering on x86-64, so this doesn't actually add any additional cost. Change-Id: I65d566270ae5fa0602fe0a907e78c2b6ae227353 Reviewed-on: https://review.couchbase.org/c/kv_engine/+/177600 Tested-by: Build Bot Well-Formed: Restriction Checker Reviewed-by: Paolo Cocchi Reviewed-by: Ben Huddleston --- engines/ep/src/objectregistry.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/engines/ep/src/objectregistry.cc b/engines/ep/src/objectregistry.cc index 8019c21784..b881409797 100644 --- a/engines/ep/src/objectregistry.cc +++ b/engines/ep/src/objectregistry.cc @@ -32,7 +32,7 @@ extern "C" { } } -static get_allocation_size getAllocSize = defaultGetAllocSize; +static std::atomic getAllocSize{defaultGetAllocSize}; @@ -69,11 +69,11 @@ static bool verifyEngine(EventuallyPersistentEngine *engine) } void ObjectRegistry::initialize(get_allocation_size func) { - getAllocSize = func; + getAllocSize.store(func, std::memory_order_release); } void ObjectRegistry::reset() { - getAllocSize = defaultGetAllocSize; + getAllocSize.store(defaultGetAllocSize, std::memory_order_release); } void ObjectRegistry::onCreateBlob(const Blob *blob) @@ -82,7 +82,7 @@ void ObjectRegistry::onCreateBlob(const Blob *blob) if (verifyEngine(engine)) { auto& coreLocalStats = engine->getEpStats().coreLocal.get(); - size_t size = getAllocSize(blob); + size_t size = getAllocSize.load(std::memory_order_acquire)(blob); if (size == 0) { size = blob->getSize(); } else { @@ -100,7 +100,7 @@ void ObjectRegistry::onDeleteBlob(const Blob *blob) if (verifyEngine(engine)) { auto& coreLocalStats = engine->getEpStats().coreLocal.get(); - size_t size = getAllocSize(blob); + size_t size = getAllocSize.load(std::memory_order_acquire)(blob); if (size == 0) { size = blob->getSize(); } else { @@ -118,7 +118,7 @@ void ObjectRegistry::onCreateStoredValue(const StoredValue *sv) if (verifyEngine(engine)) { auto& coreLocalStats = engine->getEpStats().coreLocal.get(); - size_t size = getAllocSize(sv); + size_t size = getAllocSize.load(std::memory_order_acquire)(sv); if (size == 0) { size = sv->getObjectSize(); } @@ -133,7 +133,7 @@ void ObjectRegistry::onDeleteStoredValue(const StoredValue *sv) if (verifyEngine(engine)) { auto& coreLocalStats = engine->getEpStats().coreLocal.get(); - size_t size = getAllocSize(sv); + size_t size = getAllocSize.load(std::memory_order_acquire)(sv); if (size == 0) { size = sv->getObjectSize(); }