diff --git a/cachelib/allocator/CacheAllocator-inl.h b/cachelib/allocator/CacheAllocator-inl.h index 954d533daa..baad293c4a 100644 --- a/cachelib/allocator/CacheAllocator-inl.h +++ b/cachelib/allocator/CacheAllocator-inl.h @@ -505,6 +505,18 @@ template typename CacheAllocator::WriteHandle CacheAllocator::allocateChainedItemInternal( const ReadHandle& parent, uint32_t size) { + auto tid = 0; /* TODO: consult admission policy */ + for(TierId tid = 0; tid < getNumTiers(); ++tid) { + auto handle = allocateChainedItemInternalTier(*parent, size, tid); + if (handle) return handle; + } + return {}; +} + +template +typename CacheAllocator::WriteHandle +CacheAllocator::allocateChainedItemInternalTier( + const Item& parent, uint32_t size, TierId tid) { util::LatencyTracker tracker{stats().allocateLatency_}; SCOPE_FAIL { stats_.invalidAllocs.inc(); }; @@ -512,11 +524,9 @@ CacheAllocator::allocateChainedItemInternal( // number of bytes required for this item const auto requiredSize = ChainedItem::getRequiredSize(size); - // TODO: is this correct? - auto tid = getTierId(*parent); - - const auto pid = allocator_[tid]->getAllocInfo(parent->getMemory()).poolId; - const auto cid = allocator_[tid]->getAllocationClassId(pid, requiredSize); + const auto ptid = getTierId(parent); //it is okay because pools/classes are duplicated among the tiers + const auto pid = allocator_[ptid]->getAllocInfo(parent.getMemory()).poolId; + const auto cid = allocator_[ptid]->getAllocationClassId(pid, requiredSize); util::RollingLatencyTracker rollTracker{ (*stats_.classAllocLatency)[tid][pid][cid]}; @@ -535,7 +545,7 @@ CacheAllocator::allocateChainedItemInternal( SCOPE_FAIL { allocator_[tid]->free(memory); }; auto child = acquire( - new (memory) ChainedItem(compressor_.compress(parent.getInternal()), size, + new (memory) ChainedItem(compressor_.compress(&parent), size, util::getCurrentTimeSec())); if (child) { @@ -579,7 +589,7 @@ void CacheAllocator::addChainedItem(WriteHandle& parent, // Increment refcount since this chained item is now owned by the parent // Parent will decrement the refcount upon release. Since this is an // internal refcount, we dont include it in active handle tracking. - auto ret = child->incRef(true); + auto ret = child->incRef(); XDCHECK(ret == RefcountWithFlags::incResult::incOk); XDCHECK_EQ(2u, child->getRefCount()); @@ -648,22 +658,20 @@ CacheAllocator::getParentKey(const Item& chainedItem) { } template -void CacheAllocator::transferChainLocked(WriteHandle& parent, +void CacheAllocator::transferChainLocked(Item& parent, WriteHandle& newParent) { // parent must be in a state to not have concurrent readers. Eviction code - // paths rely on holding the last item handle. Since we hold on to an item - // handle here, the chain will not be touched by any eviction code path. - XDCHECK(parent); + // paths rely on holding the last item handle. XDCHECK(newParent); - XDCHECK_EQ(parent->getKey(), newParent->getKey()); - XDCHECK(parent->hasChainedItem()); + XDCHECK_EQ(parent.getKey(), newParent->getKey()); + XDCHECK(parent.hasChainedItem()); if (newParent->hasChainedItem()) { throw std::invalid_argument(folly::sformat( "New Parent {} has invalid state", newParent->toString())); } - auto headHandle = findChainedItem(*parent); + auto headHandle = findChainedItem(parent); XDCHECK(headHandle); // remove from the access container since we are changing the key @@ -686,7 +694,7 @@ void CacheAllocator::transferChainLocked(WriteHandle& parent, folly::sformat("Did not expect to find an existing chain for {}", newParent->toString(), oldHead->toString())); } - parent->unmarkHasChainedItem(); + parent.unmarkHasChainedItem(); } template @@ -697,7 +705,7 @@ void CacheAllocator::transferChainAndReplace( } { // scope for chained item lock auto l = chainedItemLocks_.lockExclusive(parent->getKey()); - transferChainLocked(parent, newParent); + transferChainLocked(*parent, newParent); } if (replaceIfAccessible(*parent, *newParent)) { @@ -764,33 +772,10 @@ CacheAllocator::replaceChainedItem(Item& oldItem, } template -typename CacheAllocator::WriteHandle -CacheAllocator::replaceChainedItemLocked(Item& oldItem, - WriteHandle newItemHdl, - const Item& parent) { - XDCHECK(newItemHdl != nullptr); - XDCHECK_GE(1u, oldItem.getRefCount()); - - // grab the handle to the old item so that we can return this. Also, we need - // to drop the refcount the parent holds on oldItem by manually calling - // decRef. To do that safely we need to have a proper outstanding handle. - auto oldItemHdl = acquire(&oldItem); - - // Replace the old chained item with new item in the MMContainer before we - // actually replace the old item in the chain - - if (!replaceChainedItemInMMContainer(oldItem, *newItemHdl)) { - // This should never happen since we currently hold an valid - // parent handle. None of its chained items can be removed - throw std::runtime_error(folly::sformat( - "chained item cannot be replaced in MM container, oldItem={}, " - "newItem={}, parent={}", - oldItem.toString(), newItemHdl->toString(), parent.toString())); - } - - XDCHECK(!oldItem.isInMMContainer()); - XDCHECK(newItemHdl->isInMMContainer()); - +void CacheAllocator::replaceInChainLocked(Item& oldItem, + WriteHandle& newItemHdl, + const Item& parent, + bool fromMoving) { auto head = findChainedItem(parent); XDCHECK(head != nullptr); XDCHECK_EQ(reinterpret_cast( @@ -819,20 +804,78 @@ CacheAllocator::replaceChainedItemLocked(Item& oldItem, oldItem.asChainedItem().getNext(compressor_), compressor_); oldItem.asChainedItem().setNext(nullptr, compressor_); - // this should not result in 0 refcount. We are bumping down the internal - // refcount. If it did, we would leak an item. - oldItem.decRef(); - XDCHECK_LT(0u, oldItem.getRefCount()) << oldItem.toString(); + // there may be a better way to tell if this was called via + // API or internal moving (replacedChainedItemLockedForMoving) + if (fromMoving) { + if (head) { + head.reset(); + XDCHECK_EQ(1u, oldItem.getRefCount()); + } + oldItem.decRef(); + XDCHECK_EQ(0u, oldItem.getRefCount()) << oldItem.toString(); + } else { + oldItem.decRef(); + XDCHECK_LT(0u, oldItem.getRefCount()) << oldItem.toString(); + } // increment refcount to indicate parent owns this similar to addChainedItem // Since this is an internal refcount, we dont include it in active handle // tracking. - auto ret = newItemHdl->incRef(true); + auto ret = newItemHdl->incRef(); XDCHECK(ret == RefcountWithFlags::incResult::incOk); +} + +template +typename CacheAllocator::WriteHandle +CacheAllocator::replaceChainedItemLocked(Item& oldItem, + WriteHandle newItemHdl, + const Item& parent) { + XDCHECK(newItemHdl != nullptr); + XDCHECK_GE(1u, oldItem.getRefCount()); + + // grab the handle to the old item so that we can return this. Also, we need + // to drop the refcount the parent holds on oldItem by manually calling + // decRef. To do that safely we need to have a proper outstanding handle. + auto oldItemHdl = acquire(&oldItem); + XDCHECK_GE(2u, oldItem.getRefCount()); + + // Replace the old chained item with new item in the MMContainer before we + // actually replace the old item in the chain + + if (!replaceChainedItemInMMContainer(oldItem, *newItemHdl)) { + // This should never happen since we currently hold an valid + // parent handle. None of its chained items can be removed + throw std::runtime_error(folly::sformat( + "chained item cannot be replaced in MM container, oldItem={}, " + "newItem={}, parent={}", + oldItem.toString(), newItemHdl->toString(), parent.toString())); + } + + XDCHECK(!oldItem.isInMMContainer()); + XDCHECK(newItemHdl->isInMMContainer()); + + replaceInChainLocked(oldItem, newItemHdl, parent, false); + return oldItemHdl; } + +template +void CacheAllocator::replaceChainedItemLockedForMoving(Item& oldItem, + WriteHandle& newItemHdl, + const Item& parent) { + XDCHECK(newItemHdl != nullptr); + XDCHECK_EQ(1u, oldItem.getRefCount()); + + // Adding the item to mmContainer has to succeed since no one can remove the item + auto& newContainer = getMMContainer(*newItemHdl); + auto mmContainerAdded = newContainer.add(*newItemHdl); + XDCHECK(mmContainerAdded); + + replaceInChainLocked(oldItem, newItemHdl, parent, true); +} + template typename CacheAllocator::ReleaseRes CacheAllocator::releaseBackToAllocator(Item& it, @@ -862,7 +905,10 @@ CacheAllocator::releaseBackToAllocator(Item& it, // memory for a chained item but has decided not to insert the chained item // to a parent item and instead drop the chained item handle. In this case, // we free the chained item directly without calling remove callback. - if (it.isChainedItem()) { + // + // Except if we are moving a chained item between tiers - + // then it == toRecycle and we will want the normal recycle path + if (it.isChainedItem() && &it != toRecycle) { if (toRecycle) { throw std::runtime_error( folly::sformat("Can not recycle a chained item {}, toRecyle", @@ -935,10 +981,10 @@ CacheAllocator::releaseBackToAllocator(Item& it, while (head) { auto next = head->getNext(compressor_); - + const auto ctid = getTierId(head); const auto childInfo = - allocator_[tid]->getAllocInfo(static_cast(head)); - (*stats_.fragmentationSize)[tid][childInfo.poolId][childInfo.classId].sub( + allocator_[ctid]->getAllocInfo(static_cast(head)); + (*stats_.fragmentationSize)[ctid][childInfo.poolId][childInfo.classId].sub( util::getFragmentation(*this, *head)); removeFromMMContainer(*head); @@ -973,7 +1019,7 @@ CacheAllocator::releaseBackToAllocator(Item& it, XDCHECK(ReleaseRes::kReleased != res); res = ReleaseRes::kRecycled; } else { - allocator_[tid]->free(head); + allocator_[ctid]->free(head); } } @@ -984,6 +1030,7 @@ CacheAllocator::releaseBackToAllocator(Item& it, } if (&it == toRecycle) { + XDCHECK_EQ(it.getRefCount(),0u); XDCHECK(ReleaseRes::kReleased != res); res = ReleaseRes::kRecycled; } else { @@ -995,8 +1042,8 @@ CacheAllocator::releaseBackToAllocator(Item& it, } template -RefcountWithFlags::incResult CacheAllocator::incRef(Item& it, bool failIfMoving) { - auto ret = it.incRef(failIfMoving); +RefcountWithFlags::incResult CacheAllocator::incRef(Item& it) { + auto ret = it.incRef(); if (ret == RefcountWithFlags::incResult::incOk) { ++handleCount_.tlStats(); } @@ -1020,11 +1067,8 @@ CacheAllocator::acquire(Item* it) { SCOPE_FAIL { stats_.numRefcountOverflow.inc(); }; - // TODO: do not block incRef for child items to avoid deadlock - const auto failIfMoving = getNumTiers() > 1 && !it->isChainedItem(); - while (true) { - auto incRes = incRef(*it, failIfMoving); + auto incRes = incRef(*it); if (LIKELY(incRes == RefcountWithFlags::incResult::incOk)) { return WriteHandle{it, *this}; } else if (incRes == RefcountWithFlags::incResult::incFailedEviction){ @@ -1303,19 +1347,6 @@ size_t CacheAllocator::wakeUpWaitersLocked(folly::StringPiece key, template bool CacheAllocator::moveRegularItemWithSync( Item& oldItem, WriteHandle& newItemHdl) { - //on function exit - the new item handle is no longer moving - //and other threads may access it - but in case where - //we failed to replace in access container we can give the - //new item back to the allocator - auto guard = folly::makeGuard([&]() { - auto ref = newItemHdl->unmarkMoving(); - if (UNLIKELY(ref == 0)) { - const auto res = - releaseBackToAllocator(*newItemHdl, RemoveContext::kNormal, false); - XDCHECK(res == ReleaseRes::kReleased); - } - }); - XDCHECK(oldItem.isMoving()); XDCHECK(!oldItem.isExpired()); // TODO: should we introduce new latency tracker. E.g. evictRegularLatency_ @@ -1330,38 +1361,6 @@ bool CacheAllocator::moveRegularItemWithSync( newItemHdl->markNvmClean(); } - // mark new item as moving to block readers until the data is copied - // (moveCb is called). Mark item in MMContainer temporarily (TODO: should - // we remove markMoving requirement for the item to be linked?) - newItemHdl->markInMMContainer(); - auto marked = newItemHdl->markMoving(false /* there is already a handle */); - newItemHdl->unmarkInMMContainer(); - XDCHECK(marked); - - auto predicate = [&](const Item& item){ - // we rely on moving flag being set (it should block all readers) - XDCHECK(item.getRefCount() == 0); - return true; - }; - - auto replaced = accessContainer_->replaceIf(oldItem, *newItemHdl, - predicate); - // another thread may have called insertOrReplace which could have - // marked this item as unaccessible causing the replaceIf - // in the access container to fail - in this case we want - // to abort the move since the item is no longer valid - if (!replaced) { - return false; - } - // what if another thread calls insertOrReplace now when - // the item is moving and already replaced in the hash table? - // 1. it succeeds in updating the hash table - so there is - // no guarentee that isAccessible() is true - // 2. it will then try to remove from MM container - // - this operation will wait for newItemHdl to - // be unmarkedMoving via the waitContext - // 3. replaced handle is returned and eventually drops - // ref to 0 and the item is recycled back to allocator. if (config_.moveCb) { // Execute the move callback. We cannot make any guarantees about the @@ -1383,16 +1382,13 @@ bool CacheAllocator::moveRegularItemWithSync( XDCHECK(mmContainerAdded); // no one can add or remove chained items at this point + // TODO: is this race? new Item is already accessible but not yet marked with hasChainedItem() + // Should we move chainedItemLocks_.lockExclusive before replaceIf? if (oldItem.hasChainedItem()) { - // safe to acquire handle for a moving Item - auto incRes = incRef(oldItem, false); - XDCHECK(incRes == RefcountWithFlags::incResult::incOk); - auto oldHandle = WriteHandle{&oldItem,*this}; - XDCHECK_EQ(1u, oldHandle->getRefCount()) << oldHandle->toString(); XDCHECK(!newItemHdl->hasChainedItem()) << newItemHdl->toString(); try { auto l = chainedItemLocks_.lockExclusive(oldItem.getKey()); - transferChainLocked(oldHandle, newItemHdl); + transferChainLocked(oldItem, newItemHdl); } catch (const std::exception& e) { // this should never happen because we drained all the handles. XLOGF(DFATAL, "{}", e.what()); @@ -1402,119 +1398,43 @@ bool CacheAllocator::moveRegularItemWithSync( XDCHECK(!oldItem.hasChainedItem()); XDCHECK(newItemHdl->hasChainedItem()); } - newItemHdl.unmarkNascent(); - return true; -} - -template -bool CacheAllocator::moveRegularItem(Item& oldItem, - WriteHandle& newItemHdl) { - XDCHECK(config_.moveCb); - util::LatencyTracker tracker{stats_.moveRegularLatency_}; - - if (!oldItem.isAccessible() || oldItem.isExpired()) { - return false; - } - - XDCHECK_EQ(newItemHdl->getSize(), oldItem.getSize()); - XDCHECK_EQ(reinterpret_cast(&getMMContainer(oldItem)), - reinterpret_cast(&getMMContainer(*newItemHdl))); - - // take care of the flags before we expose the item to be accessed. this - // will ensure that when another thread removes the item from RAM, we issue - // a delete accordingly. See D7859775 for an example - if (oldItem.isNvmClean()) { - newItemHdl->markNvmClean(); - } - - // Execute the move callback. We cannot make any guarantees about the - // consistency of the old item beyond this point, because the callback can - // do more than a simple memcpy() e.g. update external references. If there - // are any remaining handles to the old item, it is the caller's - // responsibility to invalidate them. The move can only fail after this - // statement if the old item has been removed or replaced, in which case it - // should be fine for it to be left in an inconsistent state. - config_.moveCb(oldItem, *newItemHdl, nullptr); - - // Inside the access container's lock, this checks if the old item is - // accessible and its refcount is one. If the item is not accessible, - // there is no point to replace it since it had already been removed - // or in the process of being removed. If the item is in cache but the - // refcount is non-one, it means user could be attempting to remove - // this item through an API such as remove(itemHandle). In this case, - // it is unsafe to replace the old item with a new one, so we should - // also abort. - if (!accessContainer_->replaceIf(oldItem, *newItemHdl, - itemSlabMovePredicate)) { - return false; - } - - // Inside the MM container's lock, this checks if the old item exists to - // make sure that no other thread removed it, and only then replaces it. - if (!replaceInMMContainer(oldItem, *newItemHdl)) { - accessContainer_->remove(*newItemHdl); - return false; - } - // Replacing into the MM container was successful, but someone could have - // called insertOrReplace() or remove() before or after the - // replaceInMMContainer() operation, which would invalidate newItemHdl. - if (!newItemHdl->isAccessible()) { - removeFromMMContainer(*newItemHdl); + auto predicate = [&](const Item& item){ + // we rely on moving flag being set (it should block all readers) + XDCHECK(item.getRefCount() == 0); + return true; + }; + if (!accessContainer_->replaceIf(oldItem, *newItemHdl, predicate)) { return false; } - // no one can add or remove chained items at this point - if (oldItem.hasChainedItem()) { - auto oldItemHdl = acquire(&oldItem); - XDCHECK_EQ(1u, oldItemHdl->getRefCount()) << oldItemHdl->toString(); - XDCHECK(!newItemHdl->hasChainedItem()) << newItemHdl->toString(); - try { - auto l = chainedItemLocks_.lockExclusive(oldItem.getKey()); - transferChainLocked(oldItemHdl, newItemHdl); - } catch (const std::exception& e) { - // this should never happen because we drained all the handles. - XLOGF(DFATAL, "{}", e.what()); - throw; - } - - XDCHECK(!oldItem.hasChainedItem()); - XDCHECK(newItemHdl->hasChainedItem()); - } newItemHdl.unmarkNascent(); return true; } template -bool CacheAllocator::moveChainedItem(ChainedItem& oldItem, - WriteHandle& newItemHdl) { - XDCHECK(config_.moveCb); +bool CacheAllocator::moveChainedItemWithSync(ChainedItem& oldItem, + WriteHandle& newItemHdl, + WriteHandle& parentHandle) { util::LatencyTracker tracker{stats_.moveChainedLatency_}; - // This item has been unlinked from its parent and we're the only - // owner of it, so we're done here - if (!oldItem.isInMMContainer() || oldItem.isOnlyMoving()) { - return false; - } - auto& expectedParent = oldItem.getParentItem(compressor_); const auto parentKey = expectedParent.getKey(); - auto l = chainedItemLocks_.lockExclusive(parentKey); + auto l = chainedItemLocks_.tryLockExclusive(parentKey); // verify old item under the lock - auto parentHandle = - validateAndGetParentHandleForChainedMoveLocked(oldItem, parentKey); - if (!parentHandle || &expectedParent != parentHandle.get()) { + if (!l || !parentHandle || &expectedParent != parentHandle.get()) { return false; } - // once we have the moving sync and valid parent for the old item, check if + // check if // the original allocation was made correctly. If not, we destroy the // allocation to indicate a retry to moving logic above. if (reinterpret_cast( &newItemHdl->asChainedItem().getParentItem(compressor_)) != reinterpret_cast(&parentHandle->asChainedItem())) { newItemHdl.reset(); + XDCHECK(false); return false; } @@ -1522,27 +1442,33 @@ bool CacheAllocator::moveChainedItem(ChainedItem& oldItem, &newItemHdl->asChainedItem().getParentItem(compressor_)), reinterpret_cast(&parentHandle->asChainedItem())); - // In case someone else had removed this chained item from its parent by now - // So we check again to see if the it has been unlinked from its parent - if (!oldItem.isInMMContainer() || oldItem.isOnlyMoving()) { - return false; - } - auto parentPtr = parentHandle.getInternal(); XDCHECK_EQ(reinterpret_cast(parentPtr), reinterpret_cast(&oldItem.getParentItem(compressor_))); - // Invoke the move callback to fix up any user data related to the chain - config_.moveCb(oldItem, *newItemHdl, parentPtr); + if (config_.moveCb) { + // Execute the move callback. We cannot make any guarantees about the + // consistency of the old item beyond this point, because the callback can + // do more than a simple memcpy() e.g. update external references. If there + // are any remaining handles to the old item, it is the caller's + // responsibility to invalidate them. The move can only fail after this + // statement if the old item has been removed or replaced, in which case it + // should be fine for it to be left in an inconsistent state. + config_.moveCb(oldItem, *newItemHdl, parentPtr); + } else { + std::memcpy(newItemHdl->getMemory(), oldItem.getMemory(), + oldItem.getSize()); + } // Replace the new item in the position of the old one before both in the // parent's chain and the MMContainer. - auto oldItemHandle = - replaceChainedItemLocked(oldItem, std::move(newItemHdl), *parentHandle); - XDCHECK(oldItemHandle->isMoving()); - XDCHECK(!oldItemHandle->isInMMContainer()); - + XDCHECK_EQ(parentHandle->getRefCount(),1); + XDCHECK(parentHandle->isMoving()); + XDCHECK(l); + replaceChainedItemLockedForMoving(oldItem, + newItemHdl, + *parentHandle); return true; } @@ -1579,10 +1505,15 @@ CacheAllocator::findEviction(TierId tid, PoolId pid, ClassId cid) { config_.evictionSearchTries > searchTries)) { Item* toRecycle = nullptr; + Item* toRecycleParent = nullptr; Item* candidate = nullptr; + Item* syncItem = nullptr; typename NvmCacheT::PutToken token; + bool chainedItem = false; - mmContainer.withEvictionIterator([this, tid, pid, cid, &candidate, &toRecycle, + mmContainer.withEvictionIterator([this, tid, pid, cid, &candidate, + &toRecycle, &toRecycleParent, &syncItem, + &chainedItem, &searchTries, &mmContainer, &lastTier, &token](auto&& itr) { if (!itr) { @@ -1596,13 +1527,25 @@ CacheAllocator::findEviction(TierId tid, PoolId pid, ClassId cid) { itr) { ++searchTries; (*stats_.evictionAttempts)[tid][pid][cid].inc(); - - auto* toRecycle_ = itr.get(); - auto* candidate_ = - toRecycle_->isChainedItem() + Item* toRecycle_ = itr.get(); + bool chainedItem_ = toRecycle_->isChainedItem(); + Item* toRecycleParent_ = + chainedItem_ ? &toRecycle_->asChainedItem().getParentItem(compressor_) - : toRecycle_; - + : nullptr; + Item* candidate_; + Item* syncItem_; + //sync on the parent item for chained items to move to next tier + if (!lastTier && chainedItem_) { + syncItem_ = toRecycleParent_; + candidate_ = toRecycle_; + } else if (lastTier && chainedItem_) { + candidate_ = toRecycleParent_; + syncItem_ = toRecycleParent_; + } else { + candidate_ = toRecycle_; + syncItem_ = toRecycle_; + } if (lastTier) { // if it's last tier, the item will be evicted // need to create put token before marking it exclusive @@ -1611,23 +1554,41 @@ CacheAllocator::findEviction(TierId tid, PoolId pid, ClassId cid) { if (lastTier && shouldWriteToNvmCache(*candidate_) && !token.isValid()) { stats_.evictFailConcurrentFill.inc(); - } else if ( (lastTier && candidate_->markForEviction()) || - (!lastTier && candidate_->markMoving(true)) ) { - XDCHECK(candidate_->isMoving() || candidate_->isMarkedForEviction()); + } else if ( (lastTier && syncItem_->markForEviction()) || + (!lastTier && syncItem_->markMoving()) ) { + XDCHECK(syncItem_->isMoving() || syncItem_->isMarkedForEviction()); + toRecycleParent = toRecycleParent_; + chainedItem = chainedItem_; // markForEviction to make sure no other thead is evicting the item // nor holding a handle to that item if this is last tier // since we won't be moving the item to the next tier toRecycle = toRecycle_; candidate = candidate_; - // Check if parent changed for chained items - if yes, we cannot // remove the child from the mmContainer as we will not be evicting // it. We could abort right here, but we need to cleanup in case // unmarkForEviction() returns 0 - so just go through normal path. - if (!toRecycle_->isChainedItem() || - &toRecycle->asChainedItem().getParentItem(compressor_) == - candidate) + if (!chainedItem || + &toRecycle_->asChainedItem().getParentItem(compressor_) == + toRecycleParent_) { mmContainer.remove(itr); + } else if (chainedItem && + &toRecycle->asChainedItem().getParentItem(compressor_) != + toRecycleParent_) { + auto ref = syncItem_->unmarkMoving(); + if (UNLIKELY(ref == 0)) { + wakeUpWaiters(*syncItem_,{}); + const auto res = + releaseBackToAllocator(*syncItem_, RemoveContext::kNormal, false); + XDCHECK(res == ReleaseRes::kReleased); + } + auto parentHandle = acquire(syncItem_); + if (parentHandle) { + wakeUpWaiters(*syncItem_,std::move(parentHandle)); + } //in case where parent handle is null that means some other thread + ++itr; + continue; + } return; } @@ -1648,10 +1609,17 @@ CacheAllocator::findEviction(TierId tid, PoolId pid, ClassId cid) { XDCHECK(toRecycle); XDCHECK(candidate); - auto evictedToNext = lastTier ? nullptr : tryEvictToNextMemoryTier(*candidate, false); if (!evictedToNext) { + //failed to move a chained item - so evict the entire chain + if (candidate->isChainedItem()) { + //candidate should be parent now + XDCHECK(toRecycleParent->isMoving()); + XDCHECK_EQ(candidate,toRecycle); + candidate = toRecycleParent; //but now we evict the chain and in + //doing so recycle the child + } //if insertOrReplace was called during move //then candidate will not be accessible (failed replace during tryEvict) // - therefore this was why we failed to @@ -1696,7 +1664,33 @@ CacheAllocator::findEviction(TierId tid, PoolId pid, ClassId cid) { XDCHECK(candidate->getKey() == evictedToNext->getKey()); (*stats_.numWritebacks)[tid][pid][cid].inc(); - wakeUpWaiters(*candidate, std::move(evictedToNext)); + if (chainedItem) { + XDCHECK(toRecycleParent->isMoving()); + XDCHECK_EQ(evictedToNext->getRefCount(),2u); + //destroy new handle as we are done and going to + //unmark parent as moving - this way ref will be + //1 or 2 (if head handle) when other threads + //get access to new item once parent is unmarked moving. + evictedToNext.reset(); + auto ref = toRecycleParent->unmarkMoving(); + if (UNLIKELY(ref == 0)) { + wakeUpWaiters(*toRecycleParent,{}); + const auto res = + releaseBackToAllocator(*toRecycleParent, RemoveContext::kNormal, false); + XDCHECK(res == ReleaseRes::kReleased); + //in this case the new item has been released and we won't be recycling it + //so we should try again + continue; + } + auto parentHandle = acquire(toRecycleParent); + if (parentHandle) { + wakeUpWaiters(*toRecycleParent,std::move(parentHandle)); + } //in case where parent handle is null that means some other thread + // would have called wakeUpWaiters with null handle and released + // parent back to allocator + } else { + wakeUpWaiters(*candidate, std::move(evictedToNext)); + } } XDCHECK(!candidate->isMarkedForEviction() && !candidate->isMoving()); @@ -1779,9 +1773,9 @@ template typename CacheAllocator::WriteHandle CacheAllocator::tryEvictToNextMemoryTier( TierId tid, PoolId pid, Item& item, bool fromBgThread) { - XDCHECK(item.isMoving()); - XDCHECK(item.getRefCount() == 0); - if(item.hasChainedItem()) return WriteHandle{}; // TODO: We do not support ChainedItem yet + if (item.isInMMContainer()) { + return WriteHandle{}; //the parent item changed + } if(item.isExpired()) { accessContainer_->remove(item); item.unmarkMoving(); @@ -1791,20 +1785,48 @@ CacheAllocator::tryEvictToNextMemoryTier( TierId nextTier = tid; // TODO - calculate this based on some admission policy while (++nextTier < getNumTiers()) { // try to evict down to the next memory tiers // allocateInternal might trigger another eviction - auto newItemHdl = allocateInternalTier(nextTier, pid, + WriteHandle newItemHdl{}; + WriteHandle parentHandle{}; + bool chainedItem = false; + if(item.isChainedItem()) { + chainedItem = true; + auto& parentItem = item.asChainedItem().getParentItem(compressor_); + if (!parentItem.isMoving()) { + XDCHECK(item.isInMMContainer()); //parent changed + return WriteHandle{}; + } + XDCHECK(item.isChainedItem() && item.getRefCount() == 1); + XDCHECK_EQ(0, parentItem.getRefCount()); + newItemHdl = + allocateChainedItemInternalTier(parentItem, + item.getSize(), nextTier); + } else { + // this assert can fail if parent changed - + // if item is no longer chained item (how does that happen??) + XDCHECK(item.isMoving()); + XDCHECK(item.getRefCount() == 0); + newItemHdl = allocateInternalTier(nextTier, pid, item.getKey(), item.getSize(), item.getCreationTime(), item.getExpiryTime(), fromBgThread); + } if (newItemHdl) { XDCHECK_EQ(newItemHdl->getSize(), item.getSize()); - if (!moveRegularItemWithSync(item, newItemHdl)) { - return WriteHandle{}; + bool moveSuccess = chainedItem + ? moveChainedItemWithSync(item.asChainedItem(), + newItemHdl, parentHandle) + : moveRegularItemWithSync(item, newItemHdl); + if (!moveSuccess) { + return WriteHandle{}; } - XDCHECK_EQ(newItemHdl->getKey(),item.getKey()); - item.unmarkMoving(); + if (!chainedItem) { + XDCHECK_EQ(newItemHdl->getKey(),item.getKey()); + item.unmarkMoving(); + } + XDCHECK(newItemHdl->isInMMContainer()); return newItemHdl; } else { return WriteHandle{}; @@ -3031,6 +3053,8 @@ void CacheAllocator::releaseSlabImpl(TierId tid, // 4. Move on to the next item if current item is freed for (auto alloc : releaseContext.getActiveAllocations()) { auto startTimeSec = util::getCurrentTimeSec(); + Item& item = *static_cast(alloc); + // Need to mark an item for release before proceeding // If we can't mark as moving, it means the item is already freed const bool isAlreadyFreed = @@ -3039,8 +3063,6 @@ void CacheAllocator::releaseSlabImpl(TierId tid, continue; } - Item& item = *static_cast(alloc); - // Try to move this item and make sure we can free the memory const bool isMoved = moveForSlabRelease(releaseContext, item, throttler); @@ -3073,7 +3095,7 @@ typename RefcountWithFlags::Value CacheAllocator::unmarkMovingAndWak template bool CacheAllocator::moveForSlabRelease( const SlabReleaseContext& ctx, Item& oldItem, util::Throttler& throttler) { - if (!config_.moveCb) { + if (!config_.moveCb || oldItem.isChainedItem()) { return false; } @@ -3086,8 +3108,10 @@ bool CacheAllocator::moveForSlabRelease( ++itemMovingAttempts) { stats_.numMoveAttempts.inc(); - // Nothing to move and the key is likely also bogus for chained items. + // Nothing to move - in the case that tryMoving failed + // for chained items we would have already evicted the entire chain. if (oldItem.isOnlyMoving()) { + XDCHECK(!oldItem.isChainedItem()); auto ret = unmarkMovingAndWakeUpWaiters(oldItem, {}); XDCHECK(ret == 0); const auto res = @@ -3229,75 +3253,25 @@ CacheAllocator::allocateNewItemForOldItem(const Item& oldItem) { template bool CacheAllocator::tryMovingForSlabRelease( Item& oldItem, WriteHandle& newItemHdl) { - // By holding onto a user-level synchronization object, we ensure moving - // a regular item or chained item is synchronized with any potential - // user-side mutation. - std::unique_ptr syncObj; - if (config_.movingSync && getNumTiers() == 1) { - // TODO: use moving-bit synchronization for single tier as well - if (!oldItem.isChainedItem()) { - syncObj = config_.movingSync(oldItem.getKey()); - } else { - // Copy the key so we have a valid key to work with if the chained - // item is still valid. - const std::string parentKey = - oldItem.asChainedItem().getParentItem(compressor_).getKey().str(); - if (oldItem.isOnlyMoving()) { - // If chained item no longer has a refcount, its parent is already - // being released, so we abort this try to moving. - return false; - } - syncObj = config_.movingSync(parentKey); - } - // We need to differentiate between the following three scenarios: - // 1. nullptr indicates no move sync required for this particular item - // 2. moveSync.isValid() == true meaning we've obtained the sync - // 3. moveSync.isValid() == false meaning we need to abort and retry - if (syncObj && !syncObj->isValid()) { - return false; - } - } - - // TODO: we can unify move*Item and move*ItemWithSync by always - // using the moving bit to block readers. - if (getNumTiers() == 1) { - return oldItem.isChainedItem() - ? moveChainedItem(oldItem.asChainedItem(), newItemHdl) - : moveRegularItem(oldItem, newItemHdl); - } else { if (oldItem.isChainedItem() || oldItem.hasChainedItem()) { // TODO: add support for chained items return false; } else { //move can fail if another thread calls insertOrReplace - //in this case oldItem is no longer valid (not accessible, + //in this case oldItem is no longer valid (not accessible, //it gets removed from MMContainer and evictForSlabRelease //will send it back to the allocator bool ret = moveRegularItemWithSync(oldItem, newItemHdl); - if (!ret) { - //we failed to move - newItemHdl was released back to allocator - //by the moveRegularItemWithSync but oldItem is not accessible - //and no longer valid - we need to clean it up here - XDCHECK(!oldItem.isAccessible()); - oldItem.markForEvictionWhenMoving(); - unlinkItemForEviction(oldItem); - wakeUpWaiters(oldItem, {}); - } else { - removeFromMMContainer(oldItem); - } + removeFromMMContainer(oldItem); return ret; } - } } template void CacheAllocator::wakeUpWaiters(Item& item, WriteHandle handle) { - // readers do not block on 'moving' items in case there is only one tier - if (getNumTiers() > 1) { - wakeUpWaitersLocked(item.getKey(), std::move(handle)); - } + wakeUpWaitersLocked(item.getKey(), std::move(handle)); } template @@ -3306,16 +3280,19 @@ void CacheAllocator::evictForSlabRelease( auto startTime = util::getCurrentTimeSec(); while (true) { - XDCHECK(item.isMoving()); + //we can't rely on an item being marked moving because + //it may have previously been a chained item stats_.numEvictionAttempts.inc(); if (shutDownInProgress_) { - auto ref = unmarkMovingAndWakeUpWaiters(item, {}); - allocator_[getTierId(item)]->abortSlabRelease(ctx); - throw exception::SlabReleaseAborted( - folly::sformat("Slab Release aborted while trying to evict" - " Item: {} Pool: {}, Class: {}.", - item.toString(), ctx.getPoolId(), ctx.getClassId())); + if (item.isMoving()) { + auto ref = unmarkMovingAndWakeUpWaiters(item, {}); + allocator_[getTierId(item)]->abortSlabRelease(ctx); + throw exception::SlabReleaseAborted( + folly::sformat("Slab Release aborted while trying to evict" + " Item: {} Pool: {}, Class: {}.", + item.toString(), ctx.getPoolId(), ctx.getClassId())); + } } throttleWith(throttler, [&] { XLOGF(WARN, @@ -3347,69 +3324,46 @@ void CacheAllocator::evictForSlabRelease( if (item.isChainedItem()) { auto& expectedParent = item.asChainedItem().getParentItem(compressor_); - if (getNumTiers() == 1) { - // TODO: unify this with multi-tier implementation - // right now, taking a chained item lock here would lead to deadlock - const std::string parentKey = expectedParent.getKey().str(); - auto l = chainedItemLocks_.lockExclusive(parentKey); - - // check if the child is still in mmContainer and the expected parent is - // valid under the chained item lock. - if (expectedParent.getKey() != parentKey || !item.isInMMContainer() || - item.isOnlyMoving() || - &expectedParent != &item.asChainedItem().getParentItem(compressor_) || - !expectedParent.isAccessible() || !expectedParent.hasChainedItem()) { - continue; - } + const std::string parentKey = expectedParent.getKey().str(); + //TODO: this might not be needed since we have already marked parent + // as moving + auto l = chainedItemLocks_.tryLockExclusive(parentKey); - // search if the child is present in the chain - { - auto parentHandle = findInternal(parentKey); - if (!parentHandle || parentHandle != &expectedParent) { - continue; - } - - ChainedItem* head = nullptr; - { // scope for the handle - auto headHandle = findChainedItem(expectedParent); - head = headHandle ? &headHandle->asChainedItem() : nullptr; - } - - bool found = false; - while (head) { - if (head == &item) { - found = true; - break; - } - head = head->getNext(compressor_); - } - - if (!found) { - continue; - } - } + if (!l) { + continue; } + XDCHECK_EQ(expectedParent.getKey(),parentKey); + XDCHECK_EQ(&expectedParent,&item.asChainedItem().getParentItem(compressor_)); + XDCHECK(expectedParent.isAccessible()); + XDCHECK(expectedParent.hasChainedItem()); evicted = &expectedParent; + XDCHECK(evicted->isMoving()); token = createPutToken(*evicted); - if (evicted->markForEviction()) { - // unmark the child so it will be freed - item.unmarkMoving(); - unlinkItemForEviction(*evicted); - // wake up any readers that wait for the move to complete - // it's safe to do now, as we have the item marked exclusive and - // no other reader can be added to the waiters list - wakeUpWaiters(*evicted, {}); - } else { - // TODO: potential deadlock with markUseful for parent item - // for now, we do not block any reader on child items but this - // should probably be fixed - continue; - } + auto ret = evicted->markForEvictionWhenMoving(); + XDCHECK(ret); + XDCHECK_EQ(&expectedParent,&item.asChainedItem().getParentItem(compressor_)); + // unmark the child so it will be freed + // TODO entire chain just gets evicted since moveForSlabRelease + // returns false + XDCHECK(!item.isMoving()); + unlinkItemForEviction(*evicted); + // wake up any readers that wait for the move to complete + // it's safe to do now, as we have the item marked exclusive and + // no other reader can be added to the waiters list + wakeUpWaiters(*evicted, {}); } else { evicted = &item; token = createPutToken(*evicted); + if (!evicted->isMoving()) { + //this previously could have been a chained + //item so we need to ensure that it is moving + if (!evicted->markMoving()) { + continue; + } + } + XDCHECK(evicted->isMoving()); if (evicted->markForEvictionWhenMoving()) { unlinkItemForEviction(*evicted); wakeUpWaiters(*evicted, {}); @@ -3437,14 +3391,10 @@ void CacheAllocator::evictForSlabRelease( const auto res = releaseBackToAllocator(*evicted, RemoveContext::kEviction, false); - if (getNumTiers() == 1) { - XDCHECK(res == ReleaseRes::kReleased); - } else { - const bool isAlreadyFreed = - !markMovingForSlabRelease(ctx, &item, throttler); - if (!isAlreadyFreed) { - continue; - } + const bool isAlreadyFreed = + !markMovingForSlabRelease(ctx, &item, throttler); + if (!isAlreadyFreed) { + continue; } return; @@ -3494,16 +3444,32 @@ bool CacheAllocator::markMovingForSlabRelease( bool itemFreed = true; bool markedMoving = false; TierId tid = getTierId(alloc); - auto numTiers = getNumTiers(); - const auto fn = [&markedMoving, &itemFreed, numTiers](void* memory) { + const auto fn = [this, &markedMoving, &itemFreed](void* memory) { // Since this callback is executed, the item is not yet freed itemFreed = false; Item* item = static_cast(memory); - // TODO: for chained items, moving bit is only used to avoid - // freeing the item prematurely - auto failIfRefNotZero = numTiers > 1 && !item->isChainedItem(); - if (item->markMoving(failIfRefNotZero)) { - markedMoving = true; + bool chainedItem_ = item->isChainedItem(); + Item* syncItem_ = chainedItem_ + ? &item->asChainedItem().getParentItem(compressor_) + : item; + if (syncItem_->markMoving()) { + if (chainedItem_ && + syncItem_ != &item->asChainedItem().getParentItem(compressor_)) { + //case where parent changed + auto ref = syncItem_->unmarkMoving(); + if (UNLIKELY(ref == 0)) { + wakeUpWaiters(*syncItem_,{}); + const auto res = + releaseBackToAllocator(*syncItem_, RemoveContext::kNormal, false); + XDCHECK(res == ReleaseRes::kReleased); + } + auto parentHandle = acquire(syncItem_); + if (parentHandle) { + wakeUpWaiters(*syncItem_,std::move(parentHandle)); + } //in case where parent handle is null that means some other thread + } else { + markedMoving = true; + } } }; diff --git a/cachelib/allocator/CacheAllocator.h b/cachelib/allocator/CacheAllocator.h index d2956795a9..291a66c937 100644 --- a/cachelib/allocator/CacheAllocator.h +++ b/cachelib/allocator/CacheAllocator.h @@ -1386,7 +1386,7 @@ class CacheAllocator : public CacheBase { private: // wrapper around Item's refcount and active handle tracking - FOLLY_ALWAYS_INLINE RefcountWithFlags::incResult incRef(Item& it, bool failIfMoving); + FOLLY_ALWAYS_INLINE RefcountWithFlags::incResult incRef(Item& it); FOLLY_ALWAYS_INLINE RefcountWithFlags::Value decRef(Item& it); // drops the refcount and if needed, frees the allocation back to the memory @@ -1544,6 +1544,25 @@ class CacheAllocator : public CacheBase { WriteHandle allocateChainedItemInternal(const ReadHandle& parent, uint32_t size); + // Allocate a chained item to a specific tier + // + // The resulting chained item does not have a parent item and + // will be freed once the handle is dropped + // + // The parent item parameter here is mainly used to find the + // correct pool to allocate memory for this chained item + // + // @param parent parent item + // @param size the size for the chained allocation + // @param tid the tier to allocate on + // + // @return handle to the chained allocation + // @throw std::invalid_argument if the size requested is invalid or + // if the item is invalid + WriteHandle allocateChainedItemInternalTier(const Item& parent, + uint32_t size, + TierId tid); + // Given an item and its parentKey, validate that the parentKey // corresponds to an item that's the parent of the supplied item. // @@ -1620,18 +1639,16 @@ class CacheAllocator : public CacheBase { // @return true If the move was completed, and the containers were updated // successfully. bool moveRegularItemWithSync(Item& oldItem, WriteHandle& newItemHdl); - - // Moves a regular item to a different slab. This should only be used during - // slab release after the item's exclusive bit has been set. The user supplied - // callback is responsible for copying the contents and fixing the semantics - // of chained item. + + // Moves a chained item to a different memory tier. // - // @param oldItem item being moved + // @param oldItem Reference to the item being moved // @param newItemHdl Reference to the handle of the new item being moved into + // @param parentHandle Reference to the handle of the parent item // // @return true If the move was completed, and the containers were updated // successfully. - bool moveRegularItem(Item& oldItem, WriteHandle& newItemHdl); + bool moveChainedItemWithSync(ChainedItem& oldItem, WriteHandle& newItemHdl, WriteHandle& parentHandle); // template class for viewAsChainedAllocs that takes either ReadHandle or // WriteHandle @@ -1644,29 +1661,12 @@ class CacheAllocator : public CacheBase { template folly::IOBuf convertToIOBufT(Handle& handle); - // Moves a chained item to a different slab. This should only be used during - // slab release after the item's exclusive bit has been set. The user supplied - // callback is responsible for copying the contents and fixing the semantics - // of chained item. - // - // Note: If we have successfully moved the old item into the new, the - // newItemHdl is reset and no longer usable by the caller. - // - // @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. - bool moveChainedItem(ChainedItem& oldItem, WriteHandle& newItemHdl); - // Transfers the chain ownership from parent to newParent. Parent // will be unmarked as having chained allocations. Parent will not be null // after calling this API. // - // Parent and NewParent must be valid handles to items with same key and - // parent must have chained items and parent handle must be the only - // outstanding handle for parent. New parent must be without any chained item + // NewParent must be valid handles to item with same key as Parent and + // Parent must have chained items. New parent must be without any chained item // handles. // // Chained item lock for the parent's key needs to be held in exclusive mode. @@ -1675,7 +1675,7 @@ class CacheAllocator : public CacheBase { // @param newParent the new parent for the chain // // @throw if any of the conditions for parent or newParent are not met. - void transferChainLocked(WriteHandle& parent, WriteHandle& newParent); + void transferChainLocked(Item& parent, WriteHandle& newParent); // replace a chained item in the existing chain. This needs to be called // with the chained item lock held exclusive @@ -1689,6 +1689,24 @@ class CacheAllocator : public CacheBase { WriteHandle newItemHdl, const Item& parent); + void replaceChainedItemLockedForMoving(Item& oldItem, + WriteHandle& newItemHdl, + const Item& parent); + + // + // Performs the actual inplace replace - it is called from + // replaceChainedItemLockedForMoving and replaceChainedItemLocked + // must hold chainedItemLock + // + // @param oldItem the item we are replacing in the chain + // @param newItem the item we are replacing it with + // @param parent the parent for the chain + // + // @return handle to the oldItem + void replaceInChainLocked(Item& oldItem, + WriteHandle& newItemHdl, + const Item& parent, + bool fromMoving); // Insert an item into MM container. The caller must hold a valid handle for // the item. // @@ -1979,7 +1997,7 @@ auto& mmContainer = getMMContainer(tid, pid, cid); throw std::runtime_error("Not supported for chained items"); } - if (candidate->markMoving(true)) { + if (candidate->markMoving()) { mmContainer.remove(itr); candidates.push_back(candidate); } else { @@ -2052,7 +2070,7 @@ auto& mmContainer = getMMContainer(tid, pid, cid); // TODO: only allow it for read-only items? // or implement mvcc - if (candidate->markMoving(true)) { + if (candidate->markMoving()) { // promotions should rarely fail since we already marked moving mmContainer.remove(itr); candidates.push_back(candidate); diff --git a/cachelib/allocator/CacheItem-inl.h b/cachelib/allocator/CacheItem-inl.h index b33c1ea28a..0028e2776a 100644 --- a/cachelib/allocator/CacheItem-inl.h +++ b/cachelib/allocator/CacheItem-inl.h @@ -238,8 +238,8 @@ bool CacheItem::markForEvictionWhenMoving() { } template -bool CacheItem::markMoving(bool failIfRefNotZero) { - return ref_.markMoving(failIfRefNotZero); +bool CacheItem::markMoving() { + return ref_.markMoving(); } template diff --git a/cachelib/allocator/CacheItem.h b/cachelib/allocator/CacheItem.h index 6728b654eb..b7ae24fe6b 100644 --- a/cachelib/allocator/CacheItem.h +++ b/cachelib/allocator/CacheItem.h @@ -312,9 +312,9 @@ class CACHELIB_PACKED_ATTR CacheItem { // // @return true on success, failure if item is marked as exclusive // @throw exception::RefcountOverflow on ref count overflow - FOLLY_ALWAYS_INLINE RefcountWithFlags::incResult incRef(bool failIfMoving) { + FOLLY_ALWAYS_INLINE RefcountWithFlags::incResult incRef() { try { - return ref_.incRef(failIfMoving); + return ref_.incRef(); } catch (exception::RefcountOverflow& e) { throw exception::RefcountOverflow( folly::sformat("{} item: {}", e.what(), toString())); @@ -381,7 +381,7 @@ class CACHELIB_PACKED_ATTR CacheItem { * Unmarking moving will also return the refcount at the moment of * unmarking. */ - bool markMoving(bool failIfRefNotZero); + bool markMoving(); RefcountWithFlags::Value unmarkMoving() noexcept; bool isMoving() const noexcept; bool isOnlyMoving() const noexcept; diff --git a/cachelib/allocator/Refcount.h b/cachelib/allocator/Refcount.h index 8251ef15ba..a18c3b57f2 100644 --- a/cachelib/allocator/Refcount.h +++ b/cachelib/allocator/Refcount.h @@ -140,9 +140,9 @@ class FOLLY_PACK_ATTR RefcountWithFlags { // @return true if refcount is bumped. false otherwise (if item is exclusive) // @throw exception::RefcountOverflow if new count would be greater than // maxCount - FOLLY_ALWAYS_INLINE incResult incRef(bool failIfMoving) { + FOLLY_ALWAYS_INLINE incResult incRef() { incResult res = incOk; - auto predicate = [failIfMoving, &res](const Value curValue) { + auto predicate = [&res](const Value curValue) { Value bitMask = getAdminRef(); const bool exlusiveBitIsSet = curValue & bitMask; @@ -151,7 +151,7 @@ class FOLLY_PACK_ATTR RefcountWithFlags { } else if (exlusiveBitIsSet && (curValue & kAccessRefMask) == 0) { res = incFailedEviction; return false; - } else if (exlusiveBitIsSet && failIfMoving) { + } else if (exlusiveBitIsSet) { res = incFailedMoving; return false; } @@ -320,12 +320,17 @@ class FOLLY_PACK_ATTR RefcountWithFlags { * * Unmarking moving does not depend on `isInMMContainer` */ - bool markMoving(bool failIfRefNotZero) { - auto predicate = [failIfRefNotZero](const Value curValue) { + bool markMoving() { + auto predicate = [](const Value curValue) { Value conditionBitMask = getAdminRef(); const bool flagSet = curValue & conditionBitMask; const bool alreadyExclusive = curValue & getAdminRef(); - if (failIfRefNotZero && (curValue & kAccessRefMask) != 0) { + const bool isChained = curValue & getFlag(); + + // chained item can have ref count == 1, this just means it's linked in the chain + if (isChained && (curValue & kAccessRefMask) > 1) { + return false; + } else if (!isChained && (curValue & kAccessRefMask) != 0) { return false; } if (!flagSet || alreadyExclusive) { diff --git a/cachelib/allocator/tests/ItemTest.cpp b/cachelib/allocator/tests/ItemTest.cpp index 2f25cc07f0..54bac1945a 100644 --- a/cachelib/allocator/tests/ItemTest.cpp +++ b/cachelib/allocator/tests/ItemTest.cpp @@ -85,7 +85,7 @@ TEST(ItemTest, ExpiryTime) { // So that exclusive bit will be set item->markAccessible(); // Test that writes fail while the item is moving - result = item->markMoving(true); + result = item->markMoving(); EXPECT_TRUE(result); result = item->updateExpiryTime(0); EXPECT_FALSE(result); diff --git a/cachelib/allocator/tests/RefCountTest.cpp b/cachelib/allocator/tests/RefCountTest.cpp index 862271b03d..8388d8b45d 100644 --- a/cachelib/allocator/tests/RefCountTest.cpp +++ b/cachelib/allocator/tests/RefCountTest.cpp @@ -52,7 +52,7 @@ void RefCountTest::testMultiThreaded() { nLocalRef--; ref.markAccessible(); } else { - ref.incRef(true); + ref.incRef(); nLocalRef++; ref.unmarkAccessible(); } @@ -101,12 +101,12 @@ void RefCountTest::testBasic() { ASSERT_FALSE(ref.template isFlagSet()); for (uint32_t i = 0; i < RefcountWithFlags::kAccessRefMask; i++) { - ASSERT_EQ(ref.incRef(true),RefcountWithFlags::incOk); + ASSERT_EQ(ref.incRef(),RefcountWithFlags::incOk); } // Incrementing past the max will fail auto rawRef = ref.getRaw(); - ASSERT_THROW(ref.incRef(true), std::overflow_error); + ASSERT_THROW(ref.incRef(), std::overflow_error); ASSERT_EQ(rawRef, ref.getRaw()); // Bumping up access ref shouldn't affect admin ref and flags @@ -152,11 +152,11 @@ void RefCountTest::testBasic() { ASSERT_FALSE(ref.template isFlagSet()); // conditionally set flags - ASSERT_FALSE(ref.markMoving(true)); + ASSERT_FALSE(ref.markMoving()); ref.markInMMContainer(); // only first one succeeds - ASSERT_TRUE(ref.markMoving(true)); - ASSERT_FALSE(ref.markMoving(true)); + ASSERT_TRUE(ref.markMoving()); + ASSERT_FALSE(ref.markMoving()); ref.unmarkInMMContainer(); ref.template setFlag(); @@ -202,7 +202,7 @@ void RefCountTest::testMarkForEvictionAndMoving() { ref.markInMMContainer(); ref.markAccessible(); - ASSERT_TRUE(ref.markMoving(true)); + ASSERT_TRUE(ref.markMoving()); ASSERT_FALSE(ref.markForEviction()); ref.unmarkInMMContainer(); @@ -218,7 +218,7 @@ void RefCountTest::testMarkForEvictionAndMoving() { ref.markAccessible(); ASSERT_TRUE(ref.markForEviction()); - ASSERT_FALSE(ref.markMoving(true)); + ASSERT_FALSE(ref.markMoving()); ref.unmarkInMMContainer(); ref.unmarkAccessible(); @@ -226,29 +226,13 @@ void RefCountTest::testMarkForEvictionAndMoving() { ASSERT_EQ(ret, 0); } - { - // can mark moving when ref count > 0 - RefcountWithFlags ref; - ref.markInMMContainer(); - ref.markAccessible(); - - ref.incRef(true); - - ASSERT_TRUE(ref.markMoving(false)); - - ref.unmarkInMMContainer(); - ref.unmarkAccessible(); - auto ret = ref.unmarkMoving(); - ASSERT_EQ(ret, 1); - } - { // cannot mark for eviction when ref count > 0 RefcountWithFlags ref; ref.markInMMContainer(); ref.markAccessible(); - ref.incRef(true); + ref.incRef(); ASSERT_FALSE(ref.markForEviction()); } } diff --git a/cachelib/cachebench/test_configs/small_moving_bg.json b/cachelib/cachebench/test_configs/small_moving_bg.json new file mode 100644 index 0000000000..5807b86666 --- /dev/null +++ b/cachelib/cachebench/test_configs/small_moving_bg.json @@ -0,0 +1,40 @@ +// @nolint like default.json, but moves items during slab release instead of evicting them. +{ + "cache_config" : { + "cacheSizeMB" : 2248, + "cacheDir": "/tmp/mem-tier5", + "memoryTiers" : [ + { + "ratio": 1, + "memBindNodes": 0 + }, { + "ratio": 1, + "memBindNodes": 0 + } + ], + "poolRebalanceIntervalSec" : 1, + "moveOnSlabRelease" : true, + "rebalanceMinSlabs" : 2, + "evictorThreads": 2, + "promoterThreads": 2 + }, + "test_config" : + { + "preallocateCache" : true, + "numOps" : 40000000, + "numThreads" : 32, + "numKeys" : 250000, + "generator": "online", + + "keySizeRange" : [1, 8, 32, 64, 128, 256, 512], + "keySizeRangeProbability" : [0.1, 0.1, 0.2, 0.2, 0.3, 0.1], + + "valSizeRange" : [1, 128, 512, 1024, 4096, 10240, 20480, 40960, 60000], + "valSizeRangeProbability" : [0.1, 0.1, 0.1, 0.2, 0.2, 0.1, 0.1, 0.1], + + "getRatio" : 0.70, + "setRatio" : 0.30 + } + + } + \ No newline at end of file diff --git a/cachelib/common/Mutex.h b/cachelib/common/Mutex.h index 1d6e5898f1..15b440d406 100644 --- a/cachelib/common/Mutex.h +++ b/cachelib/common/Mutex.h @@ -341,6 +341,7 @@ class RWBucketLocks : public BaseBucketLocks { using Lock = LockType; using ReadLockHolder = ReadLockHolderType; using WriteLockHolder = WriteLockHolderType; + using LockHolder = std::unique_lock; RWBucketLocks(uint32_t locksPower, std::shared_ptr hasher) : Base::BaseBucketLocks(locksPower, std::move(hasher)) {} @@ -357,6 +358,11 @@ class RWBucketLocks : public BaseBucketLocks { return WriteLockHolder{Base::getLock(args...)}; } + template + LockHolder tryLockExclusive(Args... args) noexcept { + return LockHolder(Base::getLock(args...), std::try_to_lock); + } + // try to grab the reader lock for a limit _timeout_ duration template ReadLockHolder lockShared(const std::chrono::microseconds& timeout, diff --git a/run_tests.sh b/run_tests.sh index e575dbc62a..1d68eb41b7 100755 --- a/run_tests.sh +++ b/run_tests.sh @@ -13,3 +13,4 @@ fi ../bin/cachebench --json_test_config ../test_configs/consistency/navy.json ../bin/cachebench --json_test_config ../test_configs/consistency/navy-multi-tier.json +../opt/bin/cachebench --json_test_config /opt/test_configs/small_moving_bg.json