From 0f2fe8165f44ebde7d3fc2f63ec34cda4d10ac57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Chor=C4=85=C5=BCewicz?= Date: Tue, 12 Apr 2022 07:42:13 -0400 Subject: [PATCH 1/2] Shorten critical section in findEviction Remove the item from mmContainer and drop the lock before attempting eviction. Use moving bit for synchronization in findEviction moving bit is used to give exclusive right to evict the item to a particular thread. Originially, there was an assumption that whoever marked the item as moving will try to free it until he succeeds. Since we don't want to do that in findEviction (potentially can take a long time) we need to make sure that unmarking is safe. This patch checks the flags after unmarking (atomically) and if ref is zero it also recyles the item. This is needed as there might be some concurrent thread releasing the item (and decrementing ref count). If moving bit is set, that thread would not free the memory back to allocator, resulting in memory leak on unmarkMoving(). --- cachelib/allocator/CacheAllocator-inl.h | 260 +++++++----------------- cachelib/allocator/CacheAllocator.h | 33 +-- cachelib/allocator/CacheItem-inl.h | 4 +- cachelib/allocator/CacheItem.h | 2 +- cachelib/allocator/Refcount.h | 13 +- 5 files changed, 84 insertions(+), 228 deletions(-) diff --git a/cachelib/allocator/CacheAllocator-inl.h b/cachelib/allocator/CacheAllocator-inl.h index c8c11c77f5..3948cfddd3 100644 --- a/cachelib/allocator/CacheAllocator-inl.h +++ b/cachelib/allocator/CacheAllocator-inl.h @@ -415,7 +415,7 @@ CacheAllocator::allocateInternalTier(TierId tid, } template -typename CacheAllocator::ItemHandle +typename CacheAllocator::WriteHandle CacheAllocator::allocateInternal(PoolId pid, typename Item::Key key, uint32_t size, @@ -1186,14 +1186,13 @@ bool CacheAllocator::addWaitContextForMovingItem( } template -template typename CacheAllocator::ItemHandle CacheAllocator::moveRegularItemOnEviction( - ItemPtr& oldItemPtr, ItemHandle& newItemHdl) { + Item& oldItem, ItemHandle& newItemHdl) { + XDCHECK(oldItem.isMoving()); // TODO: should we introduce new latency tracker. E.g. evictRegularLatency_ // ??? util::LatencyTracker tracker{stats_.evictRegularLatency_}; - Item& oldItem = *oldItemPtr; if (!oldItem.isAccessible() || oldItem.isExpired()) { return {}; } @@ -1249,7 +1248,7 @@ CacheAllocator::moveRegularItemOnEviction( // it is unsafe to replace the old item with a new one, so we should // also abort. if (!accessContainer_->replaceIf(oldItem, *newItemHdl, - itemEvictionPredicate)) { + itemMovingPredicate)) { return {}; } @@ -1269,7 +1268,7 @@ CacheAllocator::moveRegularItemOnEviction( // 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(oldItemPtr, *newItemHdl)) { + if (!replaceInMMContainer(oldItem, *newItemHdl)) { accessContainer_->remove(*newItemHdl); return {}; } @@ -1459,36 +1458,45 @@ CacheAllocator::findEviction(TierId tid, PoolId pid, ClassId cid) { itr) { ++searchTries; - Item* candidate = itr.get(); + Item* toRecycle = itr.get(); + + Item* candidate = + toRecycle->isChainedItem() + ? &toRecycle->asChainedItem().getParentItem(compressor_) + : toRecycle; + + // make sure no other thead is evicting the item + if (candidate->getRefCount() != 0 || !candidate->markMoving()) { + ++itr; + continue; + } + + itr.destroy(); + // for chained items, the ownership of the parent can change. We try to // evict what we think as parent and see if the eviction of parent // recycles the child we intend to. - - ItemHandle toReleaseHandle = tryEvictToNextMemoryTier(tid, pid, itr); - bool movedToNextTier = false; - if(toReleaseHandle) { - movedToNextTier = true; - } else { - toReleaseHandle = - itr->isChainedItem() - ? advanceIteratorAndTryEvictChainedItem(tid, pid, itr) - : advanceIteratorAndTryEvictRegularItem(tid, pid, mmContainer, itr); - } + auto toReleaseHandle = + evictNormalItem(*candidate, true /* skipIfTokenInvalid */); + auto ref = candidate->unmarkMoving(); - if (toReleaseHandle) { - if (toReleaseHandle->hasChainedItem()) { + if (toReleaseHandle || ref == 0u) { + if (candidate->hasChainedItem()) { (*stats_.chainedItemEvictions)[pid][cid].inc(); } else { (*stats_.regularItemEvictions)[pid][cid].inc(); } + } else { + if (candidate->hasChainedItem()) { + stats_.evictFailParentAC.inc(); + } else { + stats_.evictFailAC.inc(); + } + } - // Invalidate iterator since later on we may use this mmContainer - // again, which cannot be done unless we drop this iterator - itr.destroy(); - - // we must be the last handle and for chained items, this will be - // the parent. - XDCHECK(toReleaseHandle.get() == candidate || candidate->isChainedItem()); + if (toReleaseHandle) { + XDCHECK(toReleaseHandle.get() == candidate); + XDCHECK(toRecycle == candidate || toRecycle->isChainedItem()); XDCHECK_EQ(1u, toReleaseHandle->getRefCount()); // We manually release the item here because we don't want to @@ -1504,16 +1512,21 @@ CacheAllocator::findEviction(TierId tid, PoolId pid, ClassId cid) { // recycle the candidate. if (ReleaseRes::kRecycled == releaseBackToAllocator(itemToRelease, RemoveContext::kEviction, - /* isNascent */ movedToNextTier, candidate)) { - return candidate; + /* isNascent */ false, toRecycle)) { + return toRecycle; + } + } else if (ref == 0u) { + // it's safe to recycle the item here as there are no more + // references and the item could not been marked as moving + // by other thread since it's detached from MMContainer. + if (ReleaseRes::kRecycled == + releaseBackToAllocator(*candidate, RemoveContext::kEviction, + /* isNascent */ false, toRecycle)) { + return toRecycle; } } - // If we destroyed the itr to possibly evict and failed, we restart - // from the beginning again - if (!itr) { - itr.resetToBegin(); - } + itr.resetToBegin(); } return nullptr; } @@ -1567,24 +1580,23 @@ bool CacheAllocator::shouldWriteToNvmCacheExclusive( } template -template -typename CacheAllocator::ItemHandle +typename CacheAllocator::WriteHandle CacheAllocator::tryEvictToNextMemoryTier( - TierId tid, PoolId pid, ItemPtr& item) { - if(item->isChainedItem()) return {}; // TODO: We do not support ChainedItem yet - if(item->isExpired()) return acquire(item); + TierId tid, PoolId pid, Item& item) { + if(item.isChainedItem()) return {}; // TODO: We do not support ChainedItem yet + if(item.isExpired()) return acquire(&item); TierId nextTier = tid; // TODO - calculate this based on some admission policy while (++nextTier < numTiers_) { // try to evict down to the next memory tiers // allocateInternal might trigger another eviction auto newItemHdl = allocateInternalTier(nextTier, pid, - item->getKey(), - item->getSize(), - item->getCreationTime(), - item->getExpiryTime()); + item.getKey(), + item.getSize(), + item.getCreationTime(), + item.getExpiryTime()); if (newItemHdl) { - XDCHECK_EQ(newItemHdl->getSize(), item->getSize()); + XDCHECK_EQ(newItemHdl->getSize(), item.getSize()); return moveRegularItemOnEviction(item, newItemHdl); } @@ -1594,149 +1606,11 @@ CacheAllocator::tryEvictToNextMemoryTier( } template -typename CacheAllocator::ItemHandle -CacheAllocator::tryEvictToNextMemoryTier(Item* item) { - auto tid = getTierId(*item); - auto pid = allocator_[tid]->getAllocInfo(item->getMemory()).poolId; - return tryEvictToNextMemoryTier(tid, pid, item); -} - -template -typename CacheAllocator::ItemHandle -CacheAllocator::advanceIteratorAndTryEvictRegularItem( - TierId tid, PoolId pid, MMContainer& mmContainer, EvictionIterator& itr) { - Item& item = *itr; - - const bool evictToNvmCache = shouldWriteToNvmCache(item); - - auto token = evictToNvmCache ? nvmCache_->createPutToken(item.getKey()) - : typename NvmCacheT::PutToken{}; - // record the in-flight eviciton. If not, we move on to next item to avoid - // stalling eviction. - if (evictToNvmCache && !token.isValid()) { - ++itr; - stats_.evictFailConcurrentFill.inc(); - return ItemHandle{}; - } - - // If there are other accessors, we should abort. Acquire a handle here since - // if we remove the item from both access containers and mm containers - // below, we will need a handle to ensure proper cleanup in case we end up - // not evicting this item - auto evictHandle = accessContainer_->removeIf(item, &itemEvictionPredicate); - - if (!evictHandle) { - ++itr; - stats_.evictFailAC.inc(); - return evictHandle; - } - - mmContainer.remove(itr); - XDCHECK_EQ(reinterpret_cast(evictHandle.get()), - reinterpret_cast(&item)); - XDCHECK(!evictHandle->isInMMContainer()); - XDCHECK(!evictHandle->isAccessible()); - - // If the item is now marked as moving, that means its corresponding slab is - // being released right now. So, we look for the next item that is eligible - // for eviction. It is safe to destroy the handle here since the moving bit - // is set. Iterator was already advance by the remove call above. - if (evictHandle->isMoving()) { - stats_.evictFailMove.inc(); - return ItemHandle{}; - } - - // Invalidate iterator since later on if we are not evicting this - // item, we may need to rely on the handle we created above to ensure - // proper cleanup if the item's raw refcount has dropped to 0. - // And since this item may be a parent item that has some child items - // in this very same mmContainer, we need to make sure we drop this - // exclusive iterator so we can gain access to it when we're cleaning - // up the child items - itr.destroy(); - - // Ensure that there are no accessors after removing from the access - // container - XDCHECK(evictHandle->getRefCount() == 1); - - if (evictToNvmCache && shouldWriteToNvmCacheExclusive(item)) { - XDCHECK(token.isValid()); - nvmCache_->put(evictHandle, std::move(token)); - } - return evictHandle; -} - -template -typename CacheAllocator::ItemHandle -CacheAllocator::advanceIteratorAndTryEvictChainedItem( - TierId tid, PoolId pid, EvictionIterator& itr) { - XDCHECK(itr->isChainedItem()); - - ChainedItem* candidate = &itr->asChainedItem(); - ++itr; - - // The parent could change at any point through transferChain. However, if - // that happens, we would realize that the releaseBackToAllocator return - // kNotRecycled and we would try another chained item, leading to transient - // failure. - auto& parent = candidate->getParentItem(compressor_); - - const bool evictToNvmCache = shouldWriteToNvmCache(parent); - - auto token = evictToNvmCache ? nvmCache_->createPutToken(parent.getKey()) - : typename NvmCacheT::PutToken{}; - - // if token is invalid, return. iterator is already advanced. - if (evictToNvmCache && !token.isValid()) { - stats_.evictFailConcurrentFill.inc(); - return ItemHandle{}; - } - - // check if the parent exists in the hashtable and refcount is drained. - auto parentHandle = - accessContainer_->removeIf(parent, &itemEvictionPredicate); - if (!parentHandle) { - stats_.evictFailParentAC.inc(); - return parentHandle; - } - - // Invalidate iterator since later on we may use the mmContainer - // associated with this iterator which cannot be done unless we - // drop this iterator - // - // This must be done once we know the parent is not nullptr. - // Since we can very well be the last holder of this parent item, - // which may have a chained item that is linked in this MM container. - itr.destroy(); - - // Ensure we have the correct parent and we're the only user of the - // parent, then free it from access container. Otherwise, we abort - XDCHECK_EQ(reinterpret_cast(&parent), - reinterpret_cast(parentHandle.get())); - XDCHECK_EQ(1u, parent.getRefCount()); - - removeFromMMContainer(*parentHandle); - - XDCHECK(!parent.isInMMContainer()); - XDCHECK(!parent.isAccessible()); - - // TODO: add multi-tier support (similar as for unchained items) - - // We need to make sure the parent is not marked as moving - // and we're the only holder of the parent item. Safe to destroy the handle - // here since moving bit is set. - if (parentHandle->isMoving()) { - stats_.evictFailParentMove.inc(); - return ItemHandle{}; - } - - if (evictToNvmCache && shouldWriteToNvmCacheExclusive(*parentHandle)) { - XDCHECK(token.isValid()); - XDCHECK(parentHandle->hasChainedItem()); - nvmCache_->put(parentHandle, std::move(token)); - } - - return parentHandle; +typename CacheAllocator::WriteHandle +CacheAllocator::tryEvictToNextMemoryTier(Item& item) { + auto tid = getTierId(item); + auto pid = allocator_[tid]->getAllocInfo(item.getMemory()).poolId; + return tryEvictToNextMemoryTier(tid, pid, item); } template @@ -2936,7 +2810,7 @@ void CacheAllocator::evictForSlabRelease( auto owningHandle = item.isChainedItem() ? evictChainedItemForSlabRelease(item.asChainedItem()) - : evictNormalItemForSlabRelease(item); + : evictNormalItem(item); // we managed to evict the corresponding owner of the item and have the // last handle for the owner. @@ -2993,14 +2867,15 @@ void CacheAllocator::evictForSlabRelease( template typename CacheAllocator::ItemHandle -CacheAllocator::evictNormalItemForSlabRelease(Item& item) { +CacheAllocator::evictNormalItem(Item& item, + bool skipIfTokenInvalid) { XDCHECK(item.isMoving()); if (item.isOnlyMoving()) { return ItemHandle{}; } - auto evictHandle = tryEvictToNextMemoryTier(&item); + auto evictHandle = tryEvictToNextMemoryTier(item); if(evictHandle) return evictHandle; auto predicate = [](const Item& it) { return it.getRefCount() == 0; }; @@ -3009,6 +2884,11 @@ CacheAllocator::evictNormalItemForSlabRelease(Item& item) { auto token = evictToNvmCache ? nvmCache_->createPutToken(item.getKey()) : typename NvmCacheT::PutToken{}; + if (skipIfTokenInvalid && evictToNvmCache && !token.isValid()) { + stats_.evictFailConcurrentFill.inc(); + return ItemHandle{}; + } + // We remove the item from both access and mm containers. It doesn't matter // if someone else calls remove on the item at this moment, the item cannot // be freed as long as we have the moving bit set. diff --git a/cachelib/allocator/CacheAllocator.h b/cachelib/allocator/CacheAllocator.h index 319e66a626..fb342a6b71 100644 --- a/cachelib/allocator/CacheAllocator.h +++ b/cachelib/allocator/CacheAllocator.h @@ -1401,8 +1401,7 @@ class CacheAllocator : public CacheBase { // // @return true If the move was completed, and the containers were updated // successfully. - template - ItemHandle moveRegularItemOnEviction(ItemPtr& oldItem, ItemHandle& newItemHdl); + ItemHandle moveRegularItemOnEviction(Item& oldItem, ItemHandle& newItemHdl); // Moves a regular item to a different slab. This should only be used during // slab release after the item's moving bit has been set. The user supplied @@ -1561,25 +1560,6 @@ class CacheAllocator : public CacheBase { // @return An evicted item or nullptr if there is no suitable candidate. Item* findEviction(TierId tid, PoolId pid, ClassId cid); - // Advance the current iterator and try to evict a regular item - // - // @param mmContainer the container to look for evictions. - // @param itr iterator holding the item - // - // @return valid handle to regular item on success. This will be the last - // handle to the item. On failure an empty handle. - ItemHandle advanceIteratorAndTryEvictRegularItem(TierId tid, PoolId pid, MMContainer& mmContainer, - EvictionIterator& itr); - - // Advance the current iterator and try to evict a chained item - // Iterator may also be reset during the course of this function - // - // @param itr iterator holding the item - // - // @return valid handle to the parent item on success. This will be the last - // handle to the item - ItemHandle advanceIteratorAndTryEvictChainedItem(TierId tid, PoolId pid, EvictionIterator& itr); - // Try to move the item down to the next memory tier // // @param tid current tier ID of the item @@ -1588,8 +1568,7 @@ class CacheAllocator : public CacheBase { // // @return valid handle to the item. This will be the last // handle to the item. On failure an empty handle. - template - ItemHandle tryEvictToNextMemoryTier(TierId tid, PoolId pid, ItemPtr& item); + WriteHandle tryEvictToNextMemoryTier(TierId tid, PoolId pid, Item& item); // Try to move the item down to the next memory tier // @@ -1597,7 +1576,7 @@ class CacheAllocator : public CacheBase { // // @return valid handle to the item. This will be the last // handle to the item. On failure an empty handle. - ItemHandle tryEvictToNextMemoryTier(Item* item); + WriteHandle tryEvictToNextMemoryTier(Item& item); // Deserializer CacheAllocatorMetadata and verify the version // @@ -1724,7 +1703,7 @@ class CacheAllocator : public CacheBase { // // @return last handle for corresponding to item on success. empty handle on // failure. caller can retry if needed. - ItemHandle evictNormalItemForSlabRelease(Item& item); + ItemHandle evictNormalItem(Item& item, bool skipIfTokenInvalid = false); // Helper function to evict a child item for slab release // As a side effect, the parent item is also evicted @@ -1845,10 +1824,6 @@ class CacheAllocator : public CacheBase { return item.getRefCount() == 0; } - static bool itemEvictionPredicate(const Item& item) { - return item.getRefCount() == 0 && !item.isMoving(); - } - static bool itemExpiryPredicate(const Item& item) { return item.getRefCount() == 1 && item.isExpired(); } diff --git a/cachelib/allocator/CacheItem-inl.h b/cachelib/allocator/CacheItem-inl.h index dcdaf4444d..d26d2ac303 100644 --- a/cachelib/allocator/CacheItem-inl.h +++ b/cachelib/allocator/CacheItem-inl.h @@ -229,8 +229,8 @@ bool CacheItem::markMoving() noexcept { } template -void CacheItem::unmarkMoving() noexcept { - ref_.unmarkMoving(); +RefcountWithFlags::Value CacheItem::unmarkMoving() noexcept { + return ref_.unmarkMoving(); } template diff --git a/cachelib/allocator/CacheItem.h b/cachelib/allocator/CacheItem.h index aa660b401b..9bf3da5fcc 100644 --- a/cachelib/allocator/CacheItem.h +++ b/cachelib/allocator/CacheItem.h @@ -378,7 +378,7 @@ class CACHELIB_PACKED_ATTR CacheItem { * Unmarking moving does not depend on `isInMMContainer` */ bool markMoving() noexcept; - void unmarkMoving() noexcept; + 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 0bd604700a..cb93fb838c 100644 --- a/cachelib/allocator/Refcount.h +++ b/cachelib/allocator/Refcount.h @@ -251,10 +251,10 @@ class FOLLY_PACK_ATTR RefcountWithFlags { /** * The following four functions are used to track whether or not * an item is currently in the process of being moved. This happens during a - * slab rebalance or resize operation. + * slab rebalance or resize operation or during eviction. * - * An item can only be marked moving when `isInMMContainer` returns true. - * This operation is atomic. + * An item can only be marked moving when `isInMMContainer` returns true and + * the item is not yet marked as moving. This operation is atomic. * * User can also query if an item "isOnlyMoving". This returns true only * if the refcount is 0 and only the moving bit is set. @@ -271,7 +271,8 @@ class FOLLY_PACK_ATTR RefcountWithFlags { Value curValue = __atomic_load_n(refPtr, __ATOMIC_RELAXED); while (true) { const bool flagSet = curValue & conditionBitMask; - if (!flagSet) { + const bool alreadyMoving = curValue & bitMask; + if (!flagSet || alreadyMoving) { return false; } @@ -290,9 +291,9 @@ class FOLLY_PACK_ATTR RefcountWithFlags { } } } - void unmarkMoving() noexcept { + Value unmarkMoving() noexcept { Value bitMask = ~getAdminRef(); - __atomic_and_fetch(&refCount_, bitMask, __ATOMIC_ACQ_REL); + return __atomic_and_fetch(&refCount_, bitMask, __ATOMIC_ACQ_REL) & kRefMask; } bool isMoving() const noexcept { return getRaw() & getAdminRef(); } bool isOnlyMoving() const noexcept { From ed2af500f0f48ba328b101b9b57c4acdeb4736e7 Mon Sep 17 00:00:00 2001 From: Igor Chorazewicz Date: Mon, 13 Jun 2022 10:53:02 +0000 Subject: [PATCH 2/2] critical section inside combined_lock --- cachelib/allocator/CacheAllocator-inl.h | 43 ++++++++++++++++--------- cachelib/allocator/MM2Q-inl.h | 23 +++++++------ cachelib/allocator/MM2Q.h | 5 +++ cachelib/allocator/MMLru-inl.h | 9 ++++++ cachelib/allocator/MMLru.h | 5 +++ cachelib/allocator/MMTinyLFU-inl.h | 9 ++++++ cachelib/allocator/MMTinyLFU.h | 5 +++ 7 files changed, 71 insertions(+), 28 deletions(-) diff --git a/cachelib/allocator/CacheAllocator-inl.h b/cachelib/allocator/CacheAllocator-inl.h index 3948cfddd3..2861d1bcbc 100644 --- a/cachelib/allocator/CacheAllocator-inl.h +++ b/cachelib/allocator/CacheAllocator-inl.h @@ -1452,26 +1452,39 @@ CacheAllocator::findEviction(TierId tid, PoolId pid, ClassId cid) { // Keep searching for a candidate until we were able to evict it // or until the search limit has been exhausted unsigned int searchTries = 0; - auto itr = mmContainer.getEvictionIterator(); while ((config_.evictionSearchTries == 0 || - config_.evictionSearchTries > searchTries) && - itr) { + config_.evictionSearchTries > searchTries)) { ++searchTries; - Item* toRecycle = itr.get(); + Item* toRecycle = nullptr; + Item* candidate = nullptr; - Item* candidate = - toRecycle->isChainedItem() - ? &toRecycle->asChainedItem().getParentItem(compressor_) - : toRecycle; + mmContainer.withEvictionIterator([this, &candidate, &toRecycle, &searchTries](auto &&itr){ + while ((config_.evictionSearchTries == 0 || + config_.evictionSearchTries > searchTries) && itr) { + ++searchTries; - // make sure no other thead is evicting the item - if (candidate->getRefCount() != 0 || !candidate->markMoving()) { - ++itr; + auto *toRecycle_ = itr.get(); + auto *candidate_ = toRecycle_->isChainedItem() + ? &toRecycle_->asChainedItem().getParentItem(compressor_) + : toRecycle_; + + // make sure no other thead is evicting the item + if (candidate_->getRefCount() == 0 && candidate_->markMoving()) { + toRecycle = toRecycle_; + candidate = candidate_; + return; + } + + ++itr; + } + }); + + if (!toRecycle) continue; - } - - itr.destroy(); + + XDCHECK(toRecycle); + XDCHECK(candidate); // for chained items, the ownership of the parent can change. We try to // evict what we think as parent and see if the eviction of parent @@ -1525,8 +1538,6 @@ CacheAllocator::findEviction(TierId tid, PoolId pid, ClassId cid) { return toRecycle; } } - - itr.resetToBegin(); } return nullptr; } diff --git a/cachelib/allocator/MM2Q-inl.h b/cachelib/allocator/MM2Q-inl.h index c112f0b442..e791d6c6c3 100644 --- a/cachelib/allocator/MM2Q-inl.h +++ b/cachelib/allocator/MM2Q-inl.h @@ -238,22 +238,21 @@ MM2Q::Container::getEvictionIterator() const noexcept { // arbitrary amount of time outside a lambda-friendly piece of code (eg. they // can return the iterator from functions, pass it to functions, etc) // - // it would be theoretically possible to refactor this interface into - // something like the following to allow combining - // - // mm2q.withEvictionIterator([&](auto iterator) { - // // user code - // }); - // - // at the time of writing it is unclear if the gains from combining are - // reasonable justification for the codemod required to achieve combinability - // as we don't expect this critical section to be the hotspot in user code. - // This is however subject to change at some time in the future as and when - // this assertion becomes false. + // to get advantage of combining, use withEvictionIterator LockHolder l(*lruMutex_); return Iterator{std::move(l), lru_.rbegin()}; } +template T::*HookPtr> +template +void +MM2Q::Container::withEvictionIterator(F&& fun) { + lruMutex_->lock_combine([this, &fun]() { + fun(Iterator{LockHolder{}, lru_.rbegin()}); + }); +} + + template T::*HookPtr> void MM2Q::Container::removeLocked(T& node, bool doRebalance) noexcept { diff --git a/cachelib/allocator/MM2Q.h b/cachelib/allocator/MM2Q.h index f669192251..5138a78421 100644 --- a/cachelib/allocator/MM2Q.h +++ b/cachelib/allocator/MM2Q.h @@ -438,6 +438,11 @@ class MM2Q { // container and only one such iterator can exist at a time Iterator getEvictionIterator() const noexcept; + // Execute provided function under container lock. Function gets + // iterator passed as parameter. + template + void withEvictionIterator(F&& f); + // get the current config as a copy Config getConfig() const; diff --git a/cachelib/allocator/MMLru-inl.h b/cachelib/allocator/MMLru-inl.h index 32972f06a5..a1b8bc6961 100644 --- a/cachelib/allocator/MMLru-inl.h +++ b/cachelib/allocator/MMLru-inl.h @@ -225,6 +225,15 @@ MMLru::Container::getEvictionIterator() const noexcept { return Iterator{std::move(l), lru_.rbegin()}; } +template T::*HookPtr> +template +void +MMLru::Container::withEvictionIterator(F&& fun) { + lruMutex_->lock_combine([this, &fun]() { + fun(Iterator{LockHolder{}, lru_.rbegin()}); + }); +} + template T::*HookPtr> void MMLru::Container::ensureNotInsertionPoint(T& node) noexcept { // If we are removing the insertion point node, grow tail before we remove diff --git a/cachelib/allocator/MMLru.h b/cachelib/allocator/MMLru.h index 8c0710f9b6..d4240c8d52 100644 --- a/cachelib/allocator/MMLru.h +++ b/cachelib/allocator/MMLru.h @@ -333,6 +333,11 @@ class MMLru { // container and only one such iterator can exist at a time Iterator getEvictionIterator() const noexcept; + // Execute provided function under container lock. Function gets + // iterator passed as parameter. + template + void withEvictionIterator(F&& f); + // get copy of current config Config getConfig() const; diff --git a/cachelib/allocator/MMTinyLFU-inl.h b/cachelib/allocator/MMTinyLFU-inl.h index 9d92c7a16b..53b081062e 100644 --- a/cachelib/allocator/MMTinyLFU-inl.h +++ b/cachelib/allocator/MMTinyLFU-inl.h @@ -220,6 +220,15 @@ MMTinyLFU::Container::getEvictionIterator() const noexcept { return Iterator{std::move(l), *this}; } +template T::*HookPtr> +template +void +MMTinyLFU::Container::withEvictionIterator(F&& fun) { + LockHolder l(lruMutex_); + fun(Iterator{LockHolder{}, *this}); +} + + template T::*HookPtr> void MMTinyLFU::Container::removeLocked(T& node) noexcept { if (isTiny(node)) { diff --git a/cachelib/allocator/MMTinyLFU.h b/cachelib/allocator/MMTinyLFU.h index 863b05bf8e..c8425edf11 100644 --- a/cachelib/allocator/MMTinyLFU.h +++ b/cachelib/allocator/MMTinyLFU.h @@ -491,6 +491,11 @@ class MMTinyLFU { // container and only one such iterator can exist at a time Iterator getEvictionIterator() const noexcept; + // Execute provided function under container lock. Function gets + // iterator passed as parameter. + template + void withEvictionIterator(F&& f); + // for saving the state of the lru // // precondition: serialization must happen without any reader or writer