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 <null> <null> (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*) <null> (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 <[email protected]>
Well-Formed: Restriction Checker
Reviewed-by: Paolo Cocchi <[email protected]>
Reviewed-by: Ben Huddleston <[email protected]>

Author: daverigby

engines/ep/src/objectregistry.cc

@@ -32,7 +32,7 @@ extern "C" {
     }
 }
 
-static get_allocation_size getAllocSize = defaultGetAllocSize;
+static std::atomic<get_allocation_size> 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();
        }