diff --git a/cachelib/allocator/CacheAllocator-inl.h b/cachelib/allocator/CacheAllocator-inl.h index 00e8f45d3c..5f48c6de58 100644 --- a/cachelib/allocator/CacheAllocator-inl.h +++ b/cachelib/allocator/CacheAllocator-inl.h @@ -1308,7 +1308,7 @@ CacheAllocator::moveRegularItemWithSync( // make sure that no other thread removed it, and only then replaces it. if (!replaceInMMContainer(oldItem, *newItemHdl)) { accessContainer_->remove(*newItemHdl); - return {}; + return acquire(&oldItem); } // Replacing into the MM container was successful, but someone could have @@ -1316,7 +1316,7 @@ CacheAllocator::moveRegularItemWithSync( // replaceInMMContainer() operation, which would invalidate newItemHdl. if (!newItemHdl->isAccessible()) { removeFromMMContainer(*newItemHdl); - return {}; + return acquire(&oldItem); } // no one can add or remove chained items at this point diff --git a/cachelib/allocator/CacheAllocator.h b/cachelib/allocator/CacheAllocator.h index ca2c686cbd..9cf04cc1a9 100644 --- a/cachelib/allocator/CacheAllocator.h +++ b/cachelib/allocator/CacheAllocator.h @@ -1496,8 +1496,9 @@ class CacheAllocator : public CacheBase { // @param oldItem Reference to the item being moved // @param newItemHdl Reference to the handle of the new item being moved into // - // @return true If the move was completed, and the containers were updated - // successfully. + // @return the handle to the oldItem if the move was completed + // and the oldItem can be recycled. + // Otherwise an empty handle is returned. template WriteHandle moveRegularItemWithSync(Item& oldItem, WriteHandle& newItemHdl, P&& predicate); diff --git a/cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp b/cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp index d378522b22..0484b843f2 100644 --- a/cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp +++ b/cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp @@ -28,6 +28,8 @@ TEST_F(LruAllocatorMemoryTiersTest, MultiTiersFromFileValid) { this->testMultiTi TEST_F(LruAllocatorMemoryTiersTest, MultiTiersValidMixed) { this->testMultiTiersValidMixed(); } TEST_F(LruAllocatorMemoryTiersTest, MultiTiersNumaBindingsSysVValid) { this->testMultiTiersNumaBindingsSysVValid(); } TEST_F(LruAllocatorMemoryTiersTest, MultiTiersNumaBindingsPosixValid) { this->testMultiTiersNumaBindingsPosixValid(); } +TEST_F(LruAllocatorMemoryTiersTest, MultiTiersRemoveDuringEviction) { this->testMultiTiersRemoveDuringEviction(); } +TEST_F(LruAllocatorMemoryTiersTest, MultiTiersReplaceDuringEviction) { this->testMultiTiersReplaceDuringEviction(); } } // end of namespace tests } // end of namespace cachelib diff --git a/cachelib/allocator/tests/AllocatorMemoryTiersTest.h b/cachelib/allocator/tests/AllocatorMemoryTiersTest.h index 16e1f88728..3ff6c6a90a 100644 --- a/cachelib/allocator/tests/AllocatorMemoryTiersTest.h +++ b/cachelib/allocator/tests/AllocatorMemoryTiersTest.h @@ -20,12 +20,42 @@ #include "cachelib/allocator/MemoryTierCacheConfig.h" #include "cachelib/allocator/tests/TestBase.h" +#include + namespace facebook { namespace cachelib { namespace tests { template class AllocatorMemoryTiersTest : public AllocatorTest { + private: + template + void testMultiTiersAsyncOpDuringMove(std::unique_ptr& alloc, + PoolId& pool, bool& quit, MvCallback&& moveCb) { + typename AllocatorT::Config config; + config.setCacheSize(4 * Slab::kSize); + config.enableCachePersistence("/tmp"); + config.configureMemoryTiers({ + MemoryTierCacheConfig::fromShm() + .setRatio(1).setMemBind({0}), + MemoryTierCacheConfig::fromShm() + .setRatio(1).setMemBind({0}) + }); + + config.enableMovingOnSlabRelease(moveCb, {} /* ChainedItemsMoveSync */, + -1 /* movingAttemptsLimit */); + + alloc = std::make_unique(AllocatorT::SharedMemNew, config); + ASSERT(alloc != nullptr); + pool = alloc->addPool("default", alloc->getCacheMemoryStats().cacheSize); + + int i = 0; + while(!quit) { + auto handle = alloc->allocate(pool, std::to_string(++i), std::string("value").size()); + ASSERT(handle != nullptr); + ASSERT_NO_THROW(alloc->insertOrReplace(handle)); + } + } public: void testMultiTiersFormFileInvalid() { typename AllocatorT::Config config; @@ -124,6 +154,70 @@ class AllocatorMemoryTiersTest : public AllocatorTest { ASSERT(handle != nullptr); ASSERT_NO_THROW(alloc->insertOrReplace(handle)); } + + void testMultiTiersRemoveDuringEviction() { + std::unique_ptr alloc; + PoolId pool; + std::unique_ptr t; + folly::Latch latch(1); + bool quit = false; + + auto moveCb = [&] (typename AllocatorT::Item& oldItem, + typename AllocatorT::Item& newItem, + typename AllocatorT::Item* /* parentPtr */) { + + auto key = oldItem.getKey(); + t = std::make_unique([&](){ + // remove() function is blocked by wait context + // till item is moved to next tier. So that, we should + // notify latch before calling remove() + latch.count_down(); + alloc->remove(key); + }); + // wait till async thread is running + latch.wait(); + memcpy(newItem.getMemory(), oldItem.getMemory(), oldItem.getSize()); + quit = true; + }; + + testMultiTiersAsyncOpDuringMove(alloc, pool, quit, moveCb); + + t->join(); + } + + void testMultiTiersReplaceDuringEviction() { + std::unique_ptr alloc; + PoolId pool; + std::unique_ptr t; + folly::Latch latch(1); + bool quit = false; + + auto moveCb = [&] (typename AllocatorT::Item& oldItem, + typename AllocatorT::Item& newItem, + typename AllocatorT::Item* /* parentPtr */) { + auto key = oldItem.getKey(); + if(!quit) { + // we need to replace only once because subsequent allocate calls + // will cause evictions recursevly + quit = true; + t = std::make_unique([&](){ + auto handle = alloc->allocate(pool, key, std::string("new value").size()); + // insertOrReplace() function is blocked by wait context + // till item is moved to next tier. So that, we should + // notify latch before calling insertOrReplace() + latch.count_down(); + ASSERT_NO_THROW(alloc->insertOrReplace(handle)); + }); + // wait till async thread is running + latch.wait(); + } + memcpy(newItem.getMemory(), oldItem.getMemory(), oldItem.getSize()); + }; + + testMultiTiersAsyncOpDuringMove(alloc, pool, quit, moveCb); + + t->join(); + } }; } // namespace tests } // namespace cachelib