Skip to content

Commit b36b7ad

Browse files
byrnedjbyrnedj
committed
still locked on chain item lock
1 parent 7c53f57 commit b36b7ad

File tree

2 files changed

+83
-56
lines changed

2 files changed

+83
-56
lines changed

‎cachelib/allocator/CacheAllocator-inl.h‎

Lines changed: 82 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -1696,12 +1696,20 @@ CacheAllocator<CacheTrait>::findEviction(TierId tid, PoolId pid, ClassId cid) {
16961696
chainedItem_
16971697
? &toRecycle_->asChainedItem().getParentItem(compressor_)
16981698
: nullptr;
1699+
auto l_ = chainedItem_
1700+
? chainedItemLocks_.tryLockExclusive(toRecycleParent_->getKey())
1701+
: chainedItemLocks_.tryLockExclusive(toRecycle_->getKey());
1702+
if (chainedItem_ && !l_) {
1703+
++itr;
1704+
continue;
1705+
}
16991706
Item* candidate_;
17001707
Item* syncItem_;
17011708
//sync on the parent item for chained items to move to next tier
17021709
if (!lastTier && chainedItem_) {
17031710
syncItem_ = toRecycleParent_;
17041711
candidate_ = toRecycle_;
1712+
XDCHECK(l_);
17051713
} else if (lastTier && chainedItem_) {
17061714
candidate_ = toRecycleParent_;
17071715
syncItem_ = toRecycleParent_;
@@ -1756,7 +1764,6 @@ CacheAllocator<CacheTrait>::findEviction(TierId tid, PoolId pid, ClassId cid) {
17561764

17571765
XDCHECK(toRecycle);
17581766
XDCHECK(candidate);
1759-
17601767
auto evictedToNext = lastTier ? nullptr
17611768
: tryEvictToNextMemoryTier(*candidate, false);
17621769
if (!evictedToNext) {
@@ -1921,9 +1928,9 @@ template <typename CacheTrait>
19211928
typename CacheAllocator<CacheTrait>::WriteHandle
19221929
CacheAllocator<CacheTrait>::tryEvictToNextMemoryTier(
19231930
TierId tid, PoolId pid, Item& item, bool fromBgThread) {
1924-
XDCHECK(item.isMoving() || item.isChainedItem());
1925-
XDCHECK(item.getRefCount() == 0 ||
1926-
(item.isChainedItem() && item.getRefCount() == 1));
1931+
if (item.isInMMContainer()) {
1932+
return WriteHandle{}; //the parent item changed
1933+
}
19271934
if(item.isExpired()) {
19281935
accessContainer_->remove(item);
19291936
item.unmarkMoving();
@@ -1943,6 +1950,7 @@ CacheAllocator<CacheTrait>::tryEvictToNextMemoryTier(
19431950
XDCHECK(item.isInMMContainer()); //parent changed
19441951
return WriteHandle{};
19451952
}
1953+
XDCHECK(item.isChainedItem() && item.getRefCount() == 1);
19461954
// safe to acquire handle for a moving Item
19471955
auto incRes = incRef(*parentItem, false);
19481956
XDCHECK(incRes == RefcountWithFlags::incResult::incOk);
@@ -1952,6 +1960,10 @@ CacheAllocator<CacheTrait>::tryEvictToNextMemoryTier(
19521960
allocateChainedItemInternalTier(parentHandle,
19531961
item.getSize(), nextTier);
19541962
} else {
1963+
// this assert can fail if parent changed -
1964+
// if item is no longer chained item (how does that happen??)
1965+
XDCHECK(item.isMoving());
1966+
XDCHECK(item.getRefCount() == 0);
19551967
newItemHdl = allocateInternalTier(nextTier, pid,
19561968
item.getKey(),
19571969
item.getSize(),
@@ -3242,7 +3254,7 @@ typename RefcountWithFlags::Value CacheAllocator<CacheTrait>::unmarkMovingAndWak
32423254
template <typename CacheTrait>
32433255
bool CacheAllocator<CacheTrait>::moveForSlabRelease(
32443256
const SlabReleaseContext& ctx, Item& oldItem, util::Throttler& throttler) {
3245-
if (!config_.moveCb) {
3257+
if (!config_.moveCb || oldItem.isChainedItem()) {
32463258
return false;
32473259
}
32483260

@@ -3467,16 +3479,19 @@ void CacheAllocator<CacheTrait>::evictForSlabRelease(
34673479
auto startTime = util::getCurrentTimeSec();
34683480

34693481
while (true) {
3470-
XDCHECK(item.isMoving());
3482+
//we can't rely on an item being marked moving because
3483+
//it may have previously been a chained item
34713484
stats_.numEvictionAttempts.inc();
34723485

34733486
if (shutDownInProgress_) {
3474-
auto ref = unmarkMovingAndWakeUpWaiters(item, {});
3475-
allocator_[getTierId(item)]->abortSlabRelease(ctx);
3476-
throw exception::SlabReleaseAborted(
3477-
folly::sformat("Slab Release aborted while trying to evict"
3478-
" Item: {} Pool: {}, Class: {}.",
3479-
item.toString(), ctx.getPoolId(), ctx.getClassId()));
3487+
if (item.isMoving()) {
3488+
auto ref = unmarkMovingAndWakeUpWaiters(item, {});
3489+
allocator_[getTierId(item)]->abortSlabRelease(ctx);
3490+
throw exception::SlabReleaseAborted(
3491+
folly::sformat("Slab Release aborted while trying to evict"
3492+
" Item: {} Pool: {}, Class: {}.",
3493+
item.toString(), ctx.getPoolId(), ctx.getClassId()));
3494+
}
34803495
}
34813496
throttleWith(throttler, [&] {
34823497
XLOGF(WARN,
@@ -3508,69 +3523,85 @@ void CacheAllocator<CacheTrait>::evictForSlabRelease(
35083523
if (item.isChainedItem()) {
35093524
auto& expectedParent = item.asChainedItem().getParentItem(compressor_);
35103525

3511-
if (getNumTiers() == 1) {
3512-
// TODO: unify this with multi-tier implementation
3513-
// right now, taking a chained item lock here would lead to deadlock
3514-
const std::string parentKey = expectedParent.getKey().str();
3515-
auto l = chainedItemLocks_.lockExclusive(parentKey);
3516-
3517-
// check if the child is still in mmContainer and the expected parent is
3518-
// valid under the chained item lock.
3519-
if (expectedParent.getKey() != parentKey || !item.isInMMContainer() ||
3520-
item.isOnlyMoving() ||
3521-
&expectedParent != &item.asChainedItem().getParentItem(compressor_) ||
3522-
!expectedParent.isAccessible() || !expectedParent.hasChainedItem()) {
3526+
const std::string parentKey = expectedParent.getKey().str();
3527+
auto l = chainedItemLocks_.tryLockExclusive(parentKey);
3528+
3529+
// check if the child is still in mmContainer and the expected parent is
3530+
// valid under the chained item lock.
3531+
//if (!l) {
3532+
// continue;
3533+
//} else if (
3534+
// || expectedParent.getKey() != parentKey ||
3535+
// &expectedParent != &item.asChainedItem().getParentItem(compressor_) ||
3536+
// !expectedParent.isAccessible() || !expectedParent.hasChainedItem()) {
3537+
//}
3538+
if (!l) {
3539+
continue;
3540+
} else if (expectedParent.getKey() != parentKey) {
3541+
continue;
3542+
} else if (&expectedParent != &item.asChainedItem().getParentItem(compressor_)) {
3543+
continue;
3544+
} else if (!expectedParent.isAccessible()) {
3545+
continue;
3546+
} else if (!expectedParent.hasChainedItem()) {
3547+
continue;
3548+
}
3549+
3550+
// search if the child is present in the chain
3551+
{
3552+
auto parentHandle = findInternal(parentKey);
3553+
if (!parentHandle || parentHandle != &expectedParent) {
35233554
continue;
35243555
}
35253556

3526-
// search if the child is present in the chain
3527-
{
3528-
auto parentHandle = findInternal(parentKey);
3529-
if (!parentHandle || parentHandle != &expectedParent) {
3530-
continue;
3531-
}
3532-
3533-
ChainedItem* head = nullptr;
3534-
{ // scope for the handle
3535-
auto headHandle = findChainedItem(expectedParent);
3536-
head = headHandle ? &headHandle->asChainedItem() : nullptr;
3537-
}
3557+
ChainedItem* head = nullptr;
3558+
{ // scope for the handle
3559+
auto headHandle = findChainedItem(expectedParent);
3560+
head = headHandle ? &headHandle->asChainedItem() : nullptr;
3561+
}
35383562

3539-
bool found = false;
3540-
while (head) {
3541-
if (head == &item) {
3542-
found = true;
3543-
break;
3544-
}
3545-
head = head->getNext(compressor_);
3563+
bool found = false;
3564+
while (head) {
3565+
if (head == &item) {
3566+
found = true;
3567+
break;
35463568
}
3569+
head = head->getNext(compressor_);
3570+
}
35473571

3548-
if (!found) {
3549-
continue;
3550-
}
3572+
if (!found) {
3573+
continue;
35513574
}
35523575
}
3576+
35533577

35543578
evicted = &expectedParent;
35553579
token = createPutToken(*evicted);
35563580
if (evicted->markForEviction()) {
35573581
// unmark the child so it will be freed
3558-
item.unmarkMoving();
3582+
// TODO entire chain just gets evicted since moveForSlabRelease
3583+
// returns false
3584+
XDCHECK(!item.isMoving());
35593585
unlinkItemForEviction(*evicted);
35603586
// wake up any readers that wait for the move to complete
35613587
// it's safe to do now, as we have the item marked exclusive and
35623588
// no other reader can be added to the waiters list
35633589
wakeUpWaiters(*evicted, {});
35643590
} else {
3565-
// TODO: potential deadlock with markUseful for parent item
3566-
// for now, we do not block any reader on child items but this
3567-
// should probably be fixed
35683591
continue;
35693592
}
35703593
} else {
35713594
evicted = &item;
35723595

35733596
token = createPutToken(*evicted);
3597+
if (!evicted->isMoving()) {
3598+
//this previously could have been a chained
3599+
//item so we need to ensure that it is moving
3600+
if (!evicted->markMoving()) {
3601+
continue;
3602+
}
3603+
}
3604+
XDCHECK(evicted->isMoving());
35743605
if (evicted->markForEvictionWhenMoving()) {
35753606
unlinkItemForEviction(*evicted);
35763607
wakeUpWaiters(*evicted, {});
@@ -3655,8 +3686,7 @@ bool CacheAllocator<CacheTrait>::markMovingForSlabRelease(
36553686
bool itemFreed = true;
36563687
bool markedMoving = false;
36573688
TierId tid = getTierId(alloc);
3658-
auto numTiers = getNumTiers();
3659-
const auto fn = [&markedMoving, &itemFreed, numTiers](void* memory) {
3689+
const auto fn = [&markedMoving, &itemFreed](void* memory) {
36603690
// Since this callback is executed, the item is not yet freed
36613691
itemFreed = false;
36623692
Item* item = static_cast<Item*>(memory);

‎cachelib/allocator/Refcount.h‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -333,10 +333,7 @@ class FOLLY_PACK_ATTR RefcountWithFlags {
333333
} else if (!isChained && (curValue & kAccessRefMask) != 0) {
334334
return false;
335335
}
336-
// when we mark a chained item as moving it - it may not
337-
// be in the mmContainer
338-
if ( (!isChained && (!flagSet || alreadyExclusive)) ||
339-
( isChained && alreadyExclusive) ) {
336+
if (!flagSet || alreadyExclusive) {
340337
return false;
341338
}
342339
if (UNLIKELY((curValue & kAccessRefMask) == (kAccessRefMask))) {

0 commit comments

Comments
 (0)