Skip to content

Commit 9c10d59

Browse files
committed
Fix moveRegularItemWithSync and add tests
1 parent 829a434 commit 9c10d59

File tree

4 files changed

+95
-4
lines changed

4 files changed

+95
-4
lines changed

cachelib/allocator/CacheAllocator-inl.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1308,15 +1308,15 @@ CacheAllocator<CacheTrait>::moveRegularItemWithSync(
13081308
// make sure that no other thread removed it, and only then replaces it.
13091309
if (!replaceInMMContainer(oldItem, *newItemHdl)) {
13101310
accessContainer_->remove(*newItemHdl);
1311-
return {};
1311+
return acquire(&oldItem);
13121312
}
13131313

13141314
// Replacing into the MM container was successful, but someone could have
13151315
// called insertOrReplace() or remove() before or after the
13161316
// replaceInMMContainer() operation, which would invalidate newItemHdl.
13171317
if (!newItemHdl->isAccessible()) {
13181318
removeFromMMContainer(*newItemHdl);
1319-
return {};
1319+
return acquire(&oldItem);
13201320
}
13211321

13221322
// no one can add or remove chained items at this point

cachelib/allocator/CacheAllocator.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1496,8 +1496,9 @@ class CacheAllocator : public CacheBase {
14961496
// @param oldItem Reference to the item being moved
14971497
// @param newItemHdl Reference to the handle of the new item being moved into
14981498
//
1499-
// @return true If the move was completed, and the containers were updated
1500-
// successfully.
1499+
// @return the handle to the oldItem if the move was completed
1500+
// and the oldItem can be recycled.
1501+
// Otherwise an empty handle is returned.
15011502
template <typename P>
15021503
WriteHandle moveRegularItemWithSync(Item& oldItem, WriteHandle& newItemHdl, P&& predicate);
15031504

cachelib/allocator/tests/AllocatorMemoryTiersTest.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ TEST_F(LruAllocatorMemoryTiersTest, MultiTiersFromFileValid) { this->testMultiTi
2828
TEST_F(LruAllocatorMemoryTiersTest, MultiTiersValidMixed) { this->testMultiTiersValidMixed(); }
2929
TEST_F(LruAllocatorMemoryTiersTest, MultiTiersNumaBindingsSysVValid) { this->testMultiTiersNumaBindingsSysVValid(); }
3030
TEST_F(LruAllocatorMemoryTiersTest, MultiTiersNumaBindingsPosixValid) { this->testMultiTiersNumaBindingsPosixValid(); }
31+
TEST_F(LruAllocatorMemoryTiersTest, MultiTiersRemoveDuringEviction) { this->testMultiTiersRemoveDuringEviction(); }
32+
TEST_F(LruAllocatorMemoryTiersTest, MultiTiersReplaceDuringEviction) { this->testMultiTiersReplaceDuringEviction(); }
3133

3234
} // end of namespace tests
3335
} // end of namespace cachelib

cachelib/allocator/tests/AllocatorMemoryTiersTest.h

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,42 @@
2020
#include "cachelib/allocator/MemoryTierCacheConfig.h"
2121
#include "cachelib/allocator/tests/TestBase.h"
2222

23+
#include <folly/synchronization/Latch.h>
24+
2325
namespace facebook {
2426
namespace cachelib {
2527
namespace tests {
2628

2729
template <typename AllocatorT>
2830
class AllocatorMemoryTiersTest : public AllocatorTest<AllocatorT> {
31+
private:
32+
template<typename MvCallback>
33+
void testMultiTiersAsyncOpDuringMove(std::unique_ptr<AllocatorT>& alloc,
34+
PoolId& pool, bool& quit, MvCallback&& moveCb) {
35+
typename AllocatorT::Config config;
36+
config.setCacheSize(4 * Slab::kSize);
37+
config.enableCachePersistence("/tmp");
38+
config.configureMemoryTiers({
39+
MemoryTierCacheConfig::fromShm()
40+
.setRatio(1).setMemBind({0}),
41+
MemoryTierCacheConfig::fromShm()
42+
.setRatio(1).setMemBind({0})
43+
});
44+
45+
config.enableMovingOnSlabRelease(moveCb, {} /* ChainedItemsMoveSync */,
46+
-1 /* movingAttemptsLimit */);
47+
48+
alloc = std::make_unique<AllocatorT>(AllocatorT::SharedMemNew, config);
49+
ASSERT(alloc != nullptr);
50+
pool = alloc->addPool("default", alloc->getCacheMemoryStats().cacheSize);
51+
52+
int i = 0;
53+
while(!quit) {
54+
auto handle = alloc->allocate(pool, std::to_string(++i), std::string("value").size());
55+
ASSERT(handle != nullptr);
56+
ASSERT_NO_THROW(alloc->insertOrReplace(handle));
57+
}
58+
}
2959
public:
3060
void testMultiTiersFormFileInvalid() {
3161
typename AllocatorT::Config config;
@@ -124,6 +154,64 @@ class AllocatorMemoryTiersTest : public AllocatorTest<AllocatorT> {
124154
ASSERT(handle != nullptr);
125155
ASSERT_NO_THROW(alloc->insertOrReplace(handle));
126156
}
157+
158+
void testMultiTiersRemoveDuringEviction() {
159+
std::unique_ptr<AllocatorT> alloc;
160+
PoolId pool;
161+
std::unique_ptr<std::thread> t;
162+
folly::Latch latch(1);
163+
bool quit = false;
164+
165+
auto moveCb = [&] (typename AllocatorT::Item& oldItem,
166+
typename AllocatorT::Item& newItem,
167+
typename AllocatorT::Item* /* parentPtr */) {
168+
169+
auto key = oldItem.getKey();
170+
t = std::make_unique<std::thread>([&](){
171+
latch.count_down();
172+
alloc->remove(key);
173+
});
174+
// wait till async thread is running
175+
latch.wait();
176+
memcpy(newItem.getMemory(), oldItem.getMemory(), oldItem.getSize());
177+
quit = true;
178+
};
179+
180+
testMultiTiersAsyncOpDuringMove(alloc, pool, quit, moveCb);
181+
182+
t->join();
183+
}
184+
185+
void testMultiTiersReplaceDuringEviction() {
186+
std::unique_ptr<AllocatorT> alloc;
187+
PoolId pool;
188+
std::unique_ptr<std::thread> t;
189+
folly::Latch latch(1);
190+
bool quit = false;
191+
192+
auto moveCb = [&] (typename AllocatorT::Item& oldItem,
193+
typename AllocatorT::Item& newItem,
194+
typename AllocatorT::Item* /* parentPtr */) {
195+
auto key = oldItem.getKey();
196+
if(!quit) {
197+
// we need to replace only once because subsequent allocate calls
198+
// will cause evictions recursevly
199+
quit = true;
200+
t = std::make_unique<std::thread>([&](){
201+
auto handle = alloc->allocate(pool, key, std::string("new value").size());
202+
latch.count_down();
203+
ASSERT_NO_THROW(alloc->insertOrReplace(handle));
204+
});
205+
// wait till async thread is running
206+
latch.wait();
207+
}
208+
memcpy(newItem.getMemory(), oldItem.getMemory(), oldItem.getSize());
209+
};
210+
211+
testMultiTiersAsyncOpDuringMove(alloc, pool, quit, moveCb);
212+
213+
t->join();
214+
}
127215
};
128216
} // namespace tests
129217
} // namespace cachelib

0 commit comments

Comments
 (0)