From b9b7db37ff59fcd5a26c54802ff1c3c19ae3c6fe Mon Sep 17 00:00:00 2001 From: Igor Chorazewicz Date: Thu, 15 Dec 2022 16:26:48 +0000 Subject: [PATCH] Introduce 'markedForEviction' state for the Item. It is similar to 'moving' but requires ref count to be 0. An item which is marked for eviction causes all incRef() calls to that item to fail. This will be used to ensure that once item is selected for eviction, no one can interfere and prevent the eviction from suceeding. 'markedForEviction' relies on the same 'exlusive' bit as the 'moving' state. To distinguish between those two states, 'moving' add 1 to the refCount. This is hidden from the user, so getRefCount() will not return that extra ref. --- cachelib/allocator/CacheAllocator-inl.h | 88 ++++--- cachelib/allocator/CacheAllocator.h | 10 +- cachelib/allocator/CacheItem-inl.h | 56 ++-- cachelib/allocator/CacheItem.h | 57 ++-- cachelib/allocator/Refcount.h | 305 +++++++++++++++------- cachelib/allocator/tests/ItemTest.cpp | 14 +- cachelib/allocator/tests/RefCountTest.cpp | 102 ++++++-- 7 files changed, 450 insertions(+), 182 deletions(-) diff --git a/cachelib/allocator/CacheAllocator-inl.h b/cachelib/allocator/CacheAllocator-inl.h index d14bcfa789..ba9a88882e 100644 --- a/cachelib/allocator/CacheAllocator-inl.h +++ b/cachelib/allocator/CacheAllocator-inl.h @@ -823,20 +823,21 @@ CacheAllocator::releaseBackToAllocator(Item& it, removeFromMMContainer(*head); - // If this chained item is marked as exclusive, we will not free it. - // We must capture the exclusive state before we do the decRef when + // If this chained item is marked as moving, we will not free it. + // We must capture the moving state before we do the decRef when // we know the item must still be valid - const bool wasExclusive = head->isExclusive(); + const bool wasMoving = head->isMoving(); + XDCHECK(!head->isMarkedForEviction()); // Decref and check if we were the last reference. Now if the item - // was marked exclusive, after decRef, it will be free to be released + // was marked moving, after decRef, it will be free to be released // by slab release thread const auto childRef = head->decRef(); - // If the item is already exclusive and we already decremented the + // If the item is already moving and we already decremented the // refcount, we don't need to free this item. We'll let the slab // release thread take care of that - if (!wasExclusive) { + if (!wasMoving) { if (childRef != 0) { throw std::runtime_error(folly::sformat( "chained item refcount is not zero. We cannot proceed! " @@ -844,7 +845,7 @@ CacheAllocator::releaseBackToAllocator(Item& it, childRef, head->toString())); } - // Item is not exclusive and refcount is 0, we can proceed to + // Item is not moving and refcount is 0, we can proceed to // free it or recylce the memory if (head == toRecycle) { XDCHECK(ReleaseRes::kReleased != res); @@ -872,9 +873,12 @@ CacheAllocator::releaseBackToAllocator(Item& it, } template -void CacheAllocator::incRef(Item& it) { - it.incRef(); - ++handleCount_.tlStats(); +bool CacheAllocator::incRef(Item& it) { + if (it.incRef()) { + ++handleCount_.tlStats(); + return true; + } + return false; } template @@ -894,8 +898,12 @@ CacheAllocator::acquire(Item* it) { SCOPE_FAIL { stats_.numRefcountOverflow.inc(); }; - incRef(*it); - return WriteHandle{it, *this}; + if (LIKELY(incRef(*it))) { + return WriteHandle{it, *this}; + } else { + // item is being evicted + return WriteHandle{}; + } } template @@ -1170,7 +1178,7 @@ bool CacheAllocator::moveChainedItem(ChainedItem& oldItem, // 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.isOnlyExclusive()) { + if (!oldItem.isInMMContainer() || oldItem.isOnlyMoving()) { return false; } @@ -1201,7 +1209,7 @@ bool CacheAllocator::moveChainedItem(ChainedItem& oldItem, // 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.isOnlyExclusive()) { + if (!oldItem.isInMMContainer() || oldItem.isOnlyMoving()) { return false; } @@ -1217,7 +1225,7 @@ bool CacheAllocator::moveChainedItem(ChainedItem& oldItem, // parent's chain and the MMContainer. auto oldItemHandle = replaceChainedItemLocked(oldItem, std::move(newItemHdl), *parentHandle); - XDCHECK(oldItemHandle->isExclusive()); + XDCHECK(oldItemHandle->isMoving()); XDCHECK(!oldItemHandle->isInMMContainer()); return true; @@ -1246,7 +1254,7 @@ CacheAllocator::findEviction(PoolId pid, ClassId cid) { : toRecycle; // make sure no other thead is evicting the item - if (candidate->getRefCount() != 0 || !candidate->markExclusive()) { + if (candidate->getRefCount() != 0 || !candidate->markMoving()) { ++itr; continue; } @@ -1261,11 +1269,11 @@ CacheAllocator::findEviction(PoolId pid, ClassId cid) { ? advanceIteratorAndTryEvictChainedItem(itr) : advanceIteratorAndTryEvictRegularItem(mmContainer, itr); evictionSuccessful = toReleaseHandle != nullptr; - // destroy toReleseHandle. The item won't be released to allocator - // since we marked it as exclusive. + // destroy toReleaseHandle. The item won't be released to allocator + // since we marked for eviction. } - const auto ref = candidate->unmarkExclusive(); + const auto ref = candidate->unmarkMoving(); if (ref == 0u) { // Invalidate iterator since later on we may use this mmContainer // again, which cannot be done unless we drop this iterator @@ -2352,7 +2360,7 @@ void CacheAllocator::releaseSlabImpl( // 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 = - !markExclusiveForSlabRelease(releaseContext, alloc, throttler); + !markMovingForSlabRelease(releaseContext, alloc, throttler); if (isAlreadyFreed) { continue; } @@ -2397,8 +2405,8 @@ bool CacheAllocator::moveForSlabRelease( stats_.numMoveAttempts.inc(); // Nothing to move and the key is likely also bogus for chained items. - if (oldItem.isOnlyExclusive()) { - oldItem.unmarkExclusive(); + if (oldItem.isOnlyMoving()) { + oldItem.unmarkMoving(); const auto res = releaseBackToAllocator(oldItem, RemoveContext::kNormal, false); XDCHECK(res == ReleaseRes::kReleased); @@ -2437,7 +2445,7 @@ bool CacheAllocator::moveForSlabRelease( // that's identical to this one to replace it. Here we just need to wait // until all users have dropped the item handles before we can proceed. startTime = util::getCurrentTimeSec(); - while (!oldItem.isOnlyExclusive()) { + while (!oldItem.isOnlyMoving()) { throttleWith(throttler, [&] { XLOGF(WARN, "Spent {} seconds, slab release still waiting for refcount to " @@ -2491,8 +2499,8 @@ CacheAllocator::allocateNewItemForOldItem(const Item& oldItem) { return {}; } - // Set up the destination for the move. Since oldChainedItem would have - // the exclusive bit set, it won't be picked for eviction. + // Set up the destination for the move. Since oldChainedItem would be + // marked as moving, it won't be picked for eviction. auto newItemHdl = allocateChainedItemInternal(parentHandle, oldChainedItem.getSize()); if (!newItemHdl) { @@ -2544,7 +2552,7 @@ bool CacheAllocator::tryMovingForSlabRelease( // item is still valid. const std::string parentKey = oldItem.asChainedItem().getParentItem(compressor_).getKey().str(); - if (oldItem.isOnlyExclusive()) { + 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; @@ -2574,10 +2582,10 @@ void CacheAllocator::evictForSlabRelease( stats_.numEvictionAttempts.inc(); // if the item is already in a state where only the exclusive bit is set, - // nothing needs to be done. We simply need to unmark exclusive bit and free + // nothing needs to be done. We simply need to call unmarkMoving and free // the item. - if (item.isOnlyExclusive()) { - item.unmarkExclusive(); + if (item.isOnlyMoving()) { + item.unmarkMoving(); const auto res = releaseBackToAllocator(item, RemoveContext::kNormal, false); XDCHECK(ReleaseRes::kReleased == res); @@ -2608,7 +2616,7 @@ void CacheAllocator::evictForSlabRelease( stats_.numEvictionSuccesses.inc(); // we have the last handle. no longer need to hold on to the exclusive bit - item.unmarkExclusive(); + item.unmarkMoving(); // manually decrement the refcount to call releaseBackToAllocator const auto ref = decRef(*owningHandle); @@ -2620,7 +2628,7 @@ void CacheAllocator::evictForSlabRelease( } if (shutDownInProgress_) { - item.unmarkExclusive(); + item.unmarkMoving(); allocator_->abortSlabRelease(ctx); throw exception::SlabReleaseAborted( folly::sformat("Slab Release aborted while trying to evict" @@ -2766,9 +2774,9 @@ CacheAllocator::advanceIteratorAndTryEvictChainedItem( template typename CacheAllocator::WriteHandle CacheAllocator::evictNormalItemForSlabRelease(Item& item) { - XDCHECK(item.isExclusive()); + XDCHECK(item.isMoving()); - if (item.isOnlyExclusive()) { + if (item.isOnlyMoving()) { return WriteHandle{}; } @@ -2780,7 +2788,7 @@ CacheAllocator::evictNormalItemForSlabRelease(Item& item) { // 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 exclusive bit set. + // be freed as long as it's marked for eviction. auto handle = accessContainer_->removeIf(item, std::move(predicate)); if (!handle) { @@ -2804,7 +2812,7 @@ CacheAllocator::evictNormalItemForSlabRelease(Item& item) { template typename CacheAllocator::WriteHandle CacheAllocator::evictChainedItemForSlabRelease(ChainedItem& child) { - XDCHECK(child.isExclusive()); + XDCHECK(child.isMoving()); // We have the child marked as moving, but dont know anything about the // state of the parent. Unlike the case of regular eviction where we are @@ -2826,7 +2834,7 @@ CacheAllocator::evictChainedItemForSlabRelease(ChainedItem& child) { // check if the child is still in mmContainer and the expected parent is // valid under the chained item lock. if (expectedParent.getKey() != parentKey || !child.isInMMContainer() || - child.isOnlyExclusive() || + child.isOnlyMoving() || &expectedParent != &child.getParentItem(compressor_) || !expectedParent.isAccessible() || !expectedParent.hasChainedItem()) { return {}; @@ -2881,14 +2889,14 @@ CacheAllocator::evictChainedItemForSlabRelease(ChainedItem& child) { // In case someone else had removed this chained item from its parent by now // So we check again to see if it has been unlinked from its parent - if (!child.isInMMContainer() || child.isOnlyExclusive()) { + if (!child.isInMMContainer() || child.isOnlyMoving()) { return {}; } // check after removing from the MMContainer that the parent is still not // being marked as moving. If parent is moving, it will release the child // item and we will wait for that. - if (parentHandle->isExclusive()) { + if (parentHandle->isMoving()) { return {}; } @@ -2921,7 +2929,7 @@ bool CacheAllocator::removeIfExpired(const ReadHandle& handle) { } template -bool CacheAllocator::markExclusiveForSlabRelease( +bool CacheAllocator::markMovingForSlabRelease( const SlabReleaseContext& ctx, void* alloc, util::Throttler& throttler) { // MemoryAllocator::processAllocForRelease will execute the callback // if the item is not already free. So there are three outcomes here: @@ -2940,7 +2948,7 @@ bool CacheAllocator::markExclusiveForSlabRelease( // Since this callback is executed, the item is not yet freed itemFreed = false; Item* item = static_cast(memory); - if (item->markExclusive()) { + if (item->markMoving()) { markedMoving = true; } }; diff --git a/cachelib/allocator/CacheAllocator.h b/cachelib/allocator/CacheAllocator.h index 612f6d2185..93a85bfc38 100644 --- a/cachelib/allocator/CacheAllocator.h +++ b/cachelib/allocator/CacheAllocator.h @@ -1308,7 +1308,7 @@ class CacheAllocator : public CacheBase { private: // wrapper around Item's refcount and active handle tracking - FOLLY_ALWAYS_INLINE void incRef(Item& it); + FOLLY_ALWAYS_INLINE bool incRef(Item& it); FOLLY_ALWAYS_INLINE RefcountWithFlags::Value decRef(Item& it); // drops the refcount and if needed, frees the allocation back to the memory @@ -1756,9 +1756,9 @@ class CacheAllocator : public CacheBase { // @return true when successfully marked as moving, // fasle when this item has already been freed - bool markExclusiveForSlabRelease(const SlabReleaseContext& ctx, - void* alloc, - util::Throttler& throttler); + bool markMovingForSlabRelease(const SlabReleaseContext& ctx, + void* alloc, + util::Throttler& throttler); // "Move" (by copying) the content in this item to another memory // location by invoking the move callback. @@ -1936,7 +1936,7 @@ class CacheAllocator : public CacheBase { } static bool parentEvictForSlabReleasePredicate(const Item& item) { - return item.getRefCount() == 1 && !item.isExclusive(); + return item.getRefCount() == 1 && !item.isMoving(); } std::unique_ptr createDeserializer(); diff --git a/cachelib/allocator/CacheItem-inl.h b/cachelib/allocator/CacheItem-inl.h index f59fa9d599..bf77b43aa5 100644 --- a/cachelib/allocator/CacheItem-inl.h +++ b/cachelib/allocator/CacheItem-inl.h @@ -148,15 +148,16 @@ std::string CacheItem::toString() const { return folly::sformat( "item: " "memory={}:raw-ref={}:size={}:key={}:hex-key={}:" - "isInMMContainer={}:isAccessible={}:isExclusive={}:references={}:ctime=" + "isInMMContainer={}:isAccessible={}:isMarkedForEviction={}:" + "isMoving={}:references={}:ctime=" "{}:" "expTime={}:updateTime={}:isNvmClean={}:isNvmEvicted={}:hasChainedItem=" "{}", this, getRefCountAndFlagsRaw(), getSize(), folly::humanify(getKey().str()), folly::hexlify(getKey()), - isInMMContainer(), isAccessible(), isExclusive(), getRefCount(), - getCreationTime(), getExpiryTime(), getLastAccessTime(), isNvmClean(), - isNvmEvicted(), hasChainedItem()); + isInMMContainer(), isAccessible(), isMarkedForEviction(), isMoving(), + getRefCount(), getCreationTime(), getExpiryTime(), getLastAccessTime(), + isNvmClean(), isNvmEvicted(), hasChainedItem()); } } @@ -217,23 +218,43 @@ bool CacheItem::isInMMContainer() const noexcept { } template -bool CacheItem::markExclusive() noexcept { - return ref_.markExclusive(); +bool CacheItem::markForEviction() noexcept { + return ref_.markForEviction(); } template -RefcountWithFlags::Value CacheItem::unmarkExclusive() noexcept { - return ref_.unmarkExclusive(); +RefcountWithFlags::Value CacheItem::unmarkForEviction() noexcept { + return ref_.unmarkForEviction(); } template -bool CacheItem::isExclusive() const noexcept { - return ref_.isExclusive(); +bool CacheItem::isMarkedForEviction() const noexcept { + return ref_.isMarkedForEviction(); } template -bool CacheItem::isOnlyExclusive() const noexcept { - return ref_.isOnlyExclusive(); +bool CacheItem::markForEvictionWhenMoving() { + return ref_.markForEvictionWhenMoving(); +} + +template +bool CacheItem::markMoving() { + return ref_.markMoving(); +} + +template +RefcountWithFlags::Value CacheItem::unmarkMoving() noexcept { + return ref_.unmarkMoving(); +} + +template +bool CacheItem::isMoving() const noexcept { + return ref_.isMoving(); +} + +template +bool CacheItem::isOnlyMoving() const noexcept { + return ref_.isOnlyMoving(); } template @@ -335,7 +356,8 @@ bool CacheItem::updateExpiryTime(uint32_t expiryTimeSecs) noexcept { // check for moving to make sure we are not updating the expiry time while at // the same time re-allocating the item with the old state of the expiry time // in moveRegularItem(). See D6852328 - if (isExclusive() || !isInMMContainer() || isChainedItem()) { + if (isMoving() || isMarkedForEviction() || !isInMMContainer() || + isChainedItem()) { return false; } // attempt to atomically update the value of expiryTime @@ -451,12 +473,14 @@ std::string CacheChainedItem::toString() const { return folly::sformat( "chained item: " "memory={}:raw-ref={}:size={}:parent-compressed-ptr={}:" - "isInMMContainer={}:isAccessible={}:isExclusive={}:references={}:ctime={}" + "isInMMContainer={}:isAccessible={}:isMarkedForEviction={}:" + "isMoving={}:references={}:ctime={}" ":" "expTime={}:updateTime={}", this, Item::getRefCountAndFlagsRaw(), Item::getSize(), cPtr.getRaw(), - Item::isInMMContainer(), Item::isAccessible(), Item::isExclusive(), - Item::getRefCount(), Item::getCreationTime(), Item::getExpiryTime(), + Item::isInMMContainer(), Item::isAccessible(), + Item::isMarkedForEviction(), Item::isMoving(), Item::getRefCount(), + Item::getCreationTime(), Item::getExpiryTime(), Item::getLastAccessTime()); } diff --git a/cachelib/allocator/CacheItem.h b/cachelib/allocator/CacheItem.h index 06136db032..afee315cbb 100644 --- a/cachelib/allocator/CacheItem.h +++ b/cachelib/allocator/CacheItem.h @@ -305,12 +305,17 @@ class CACHELIB_PACKED_ATTR CacheItem { */ RefcountWithFlags::Value getRefCountAndFlagsRaw() const noexcept; - FOLLY_ALWAYS_INLINE void incRef() { - if (LIKELY(ref_.incRef())) { - return; + // Increments item's ref count + // + // @return true on success, failure if item is marked as exclusive + // @throw exception::RefcountOverflow on ref count overflow + FOLLY_ALWAYS_INLINE bool incRef() { + try { + return ref_.incRef(); + } catch (exception::RefcountOverflow& e) { + throw exception::RefcountOverflow( + folly::sformat("{} item: {}", e.what(), toString())); } - throw exception::RefcountOverflow( - folly::sformat("Refcount maxed out. item: {}", toString())); } FOLLY_ALWAYS_INLINE RefcountWithFlags::Value decRef() { @@ -344,23 +349,43 @@ class CACHELIB_PACKED_ATTR CacheItem { /** * The following two functions corresond to whether or not an item is - * currently in the process of being moved. This happens during a slab - * rebalance, eviction or resize operation. + * currently in the process of being evicted. * - * An item can only be marked exclusive when `isInMMContainer` returns true. + * An item can only be marked exclusive when `isInMMContainer` returns true + * and item is not already exclusive nor moving and the ref count is 0. * This operation is atomic. * - * User can also query if an item "isOnlyExclusive". This returns true only - * if the refcount is 0 and only the exclusive bit is set. - * - * Unmarking exclusive does not depend on `isInMMContainer`. + * Unmarking exclusive does not depend on `isInMMContainer` * Unmarking exclusive will also return the refcount at the moment of * unmarking. */ - bool markExclusive() noexcept; - RefcountWithFlags::Value unmarkExclusive() noexcept; - bool isExclusive() const noexcept; - bool isOnlyExclusive() const noexcept; + bool markForEviction() noexcept; + RefcountWithFlags::Value unmarkForEviction() noexcept; + bool isMarkedForEviction() const noexcept; + + /** + * The following functions correspond to whether or not an item is + * currently in the processed of being moved. When moving, ref count + * is always >= 1. + * + * An item can only be marked moving when `isInMMContainer` returns true + * and item is not already exclusive nor moving. + * + * User can also query if an item "isOnlyMoving". This returns true only + * if the refcount is one and only the exclusive bit is set. + * + * Unmarking moving does not depend on `isInMMContainer` + * Unmarking moving will also return the refcount at the moment of + * unmarking. + */ + bool markMoving(); + RefcountWithFlags::Value unmarkMoving() noexcept; + bool isMoving() const noexcept; + bool isOnlyMoving() const noexcept; + + /** This function attempts to mark item as exclusive. + * Can only be called on the item that is moving.*/ + bool markForEvictionWhenMoving(); /** * Item cannot be marked both chained allocation and diff --git a/cachelib/allocator/Refcount.h b/cachelib/allocator/Refcount.h index 3333762dbc..980d2c0951 100644 --- a/cachelib/allocator/Refcount.h +++ b/cachelib/allocator/Refcount.h @@ -132,32 +132,28 @@ class FOLLY_PACK_ATTR RefcountWithFlags { RefcountWithFlags& operator=(RefcountWithFlags&&) = delete; // Bumps up the reference count only if the new count will be strictly less - // than or equal to the maxCount. - // @return true if refcount is bumped. false otherwise. - FOLLY_ALWAYS_INLINE bool incRef() noexcept { - Value* const refPtr = &refCount_; - unsigned int nCASFailures = 0; - constexpr bool isWeak = false; - Value oldVal = __atomic_load_n(refPtr, __ATOMIC_RELAXED); - - while (true) { - const Value newCount = oldVal + static_cast(1); - if (UNLIKELY((oldVal & kAccessRefMask) == (kAccessRefMask))) { - return false; + // than or equal to the maxCount and the item is not exclusive + // @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 bool incRef() { + auto predicate = [](const Value curValue) { + Value bitMask = getAdminRef(); + + const bool exlusiveBitIsSet = curValue & bitMask; + if (UNLIKELY((curValue & kAccessRefMask) == (kAccessRefMask))) { + throw exception::RefcountOverflow("Refcount maxed out."); } - if (__atomic_compare_exchange_n(refPtr, &oldVal, newCount, isWeak, - __ATOMIC_ACQ_REL, __ATOMIC_RELAXED)) { - return true; - } + // Check if the item is not marked for eviction + return !exlusiveBitIsSet || ((curValue & kAccessRefMask) != 0); + }; - if ((++nCASFailures % 4) == 0) { - // this pause takes up to 40 clock cycles on intel and the lock cmpxchgl - // above should take about 100 clock cycles. we pause once every 400 - // cycles or so if we are extremely unlucky. - folly::asm_volatile_pause(); - } - } + auto newValue = [](const Value curValue) { + return (curValue + static_cast(1)); + }; + + return atomicUpdateValue(predicate, newValue); } // Bumps down the reference count @@ -167,33 +163,38 @@ class FOLLY_PACK_ATTR RefcountWithFlags { // @throw RefcountUnderflow when we are trying to decremenet from 0 // refcount and have a refcount leak. FOLLY_ALWAYS_INLINE Value decRef() { - Value* const refPtr = &refCount_; - unsigned int nCASFailures = 0; - constexpr bool isWeak = false; - - Value oldVal = __atomic_load_n(refPtr, __ATOMIC_RELAXED); - while (true) { - const Value newCount = oldVal - static_cast(1); - if ((oldVal & kAccessRefMask) == 0) { + auto predicate = [](const Value curValue) { + if ((curValue & kAccessRefMask) == 0) { throw exception::RefcountUnderflow( "Trying to decRef with no refcount. RefCount Leak!"); } + return true; + }; - if (__atomic_compare_exchange_n(refPtr, &oldVal, newCount, isWeak, - __ATOMIC_ACQ_REL, __ATOMIC_RELAXED)) { - return newCount & kRefMask; - } - if ((++nCASFailures % 4) == 0) { - // this pause takes up to 40 clock cycles on intel and the lock cmpxchgl - // above should take about 100 clock cycles. we pause once every 400 - // cycles or so if we are extremely unlucky - folly::asm_volatile_pause(); - } - } + Value retValue; + auto newValue = [&retValue](const Value curValue) { + retValue = (curValue - static_cast(1)); + return retValue; + }; + + auto updated = atomicUpdateValue(predicate, newValue); + XDCHECK(updated); + + return retValue & kRefMask; } - // Return refcount excluding control bits and flags - Value getAccessRef() const noexcept { return getRaw() & kAccessRefMask; } + // Return refcount excluding moving refcount, control bits and flags. + Value getAccessRef() const noexcept { + auto raw = getRaw(); + auto accessRef = raw & kAccessRefMask; + + if ((raw & getAdminRef()) && accessRef >= 1) { + // if item is moving, ignore the extra ref + return accessRef - static_cast(1); + } else { + return accessRef; + } + } // Return access ref and the admin ref bits Value getRefWithAccessAndAdmin() const noexcept { @@ -246,65 +247,160 @@ 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 or during eviction. + * The following two functions correspond to whether or not an item is + * currently in the process of being evicted. * - * An item can only be marked exclusive when `isInMMContainer` returns true - * and the item is not yet marked as exclusive. This operation is atomic. + * An item that is marked for eviction prevents from obtaining a handle to + * the item (incRef() will return false). This guarantees that eviction of + * marked item will always suceed. * - * User can also query if an item "isOnlyExclusive". This returns true only - * if the refcount is 0 and only the exclusive bit is set. + * An item can only be marked for eviction when `isInMMContainer` returns true + * and item does not have `kExclusive` bit set and access ref count is 0. + * This operation is atomic. * - * Unmarking exclusive does not depend on `isInMMContainer`. - * Unmarking exclusive will also return the refcount at the moment of - * unmarking. + * When item is marked for eviction, `kExclusive` bit is set and ref count is + * zero. + * + * Unmarking for eviction clears the `kExclusive` bit. `unamrkForEviction` + * does not depend on `isInMMContainer` nor `isAccessible` */ - bool markExclusive() noexcept { - Value bitMask = getAdminRef(); - Value conditionBitMask = getAdminRef(); + bool markForEviction() noexcept { + Value linkedBitMask = getAdminRef(); + Value exclusiveBitMask = getAdminRef(); - Value* const refPtr = &refCount_; - unsigned int nCASFailures = 0; - constexpr bool isWeak = false; - Value curValue = __atomic_load_n(refPtr, __ATOMIC_RELAXED); - while (true) { - const bool flagSet = curValue & conditionBitMask; - const bool alreadyExclusive = curValue & bitMask; - if (!flagSet || alreadyExclusive) { + auto predicate = [linkedBitMask, exclusiveBitMask](const Value curValue) { + const bool unlinked = !(curValue & linkedBitMask); + const bool alreadyExclusive = curValue & exclusiveBitMask; + + if (unlinked || alreadyExclusive) { return false; } - - const Value newValue = curValue | bitMask; - if (__atomic_compare_exchange_n(refPtr, &curValue, newValue, isWeak, - __ATOMIC_ACQ_REL, __ATOMIC_RELAXED)) { - XDCHECK(newValue & conditionBitMask); - return true; + if ((curValue & kAccessRefMask) != 0) { + return false; } - if ((++nCASFailures % 4) == 0) { - // this pause takes up to 40 clock cycles on intel and the lock cmpxchgl - // above should take about 100 clock cycles. we pause once every 400 - // cycles or so if we are extremely unlucky. - folly::asm_volatile_pause(); - } - } + return true; + }; + + auto newValue = [exclusiveBitMask](const Value curValue) { + return curValue | exclusiveBitMask; + }; + + return atomicUpdateValue(predicate, newValue); } - Value unmarkExclusive() noexcept { + + Value unmarkForEviction() noexcept { + XDCHECK(isMarkedForEviction()); Value bitMask = ~getAdminRef(); return __atomic_and_fetch(&refCount_, bitMask, __ATOMIC_ACQ_REL) & kRefMask; } - bool isExclusive() const noexcept { - return getRaw() & getAdminRef(); + + bool isMarkedForEviction() const noexcept { + auto raw = getRaw(); + return (raw & getAdminRef()) && ((raw & kAccessRefMask) == 0); + } + + /** + * The following functions correspond to whether or not an item is + * currently in the processed of being moved. + * + * A `moving` item cannot be recycled nor freed to the allocator. It has + * to be unmarked first. + * + * When moving, internal ref count is always >= 1 and `kExclusive` bit is set + * getRefCount does not return the extra ref (it may return 0). + * + * An item can only be marked moving when `isInMMContainer` returns true + * and does not have `kExclusive` bit set. + * + * User can also query if an item "isOnlyMoving". This returns true only + * if the refcount is one and only the exlusive bit is set. + * + * Unmarking clears `kExclusive` bit and decreses the interanl refCount by 1. + * `unmarkMoving` does does not depend on `isInMMContainer` + */ + bool markMoving() { + Value linkedBitMask = getAdminRef(); + Value exclusiveBitMask = getAdminRef(); + + auto predicate = [linkedBitMask, exclusiveBitMask](const Value curValue) { + const bool unlinked = !(curValue & linkedBitMask); + const bool alreadyExclusive = curValue & exclusiveBitMask; + + if (unlinked || alreadyExclusive) { + return false; + } + if (UNLIKELY((curValue & kAccessRefMask) == (kAccessRefMask))) { + throw exception::RefcountOverflow("Refcount maxed out."); + } + + return true; + }; + + auto newValue = [exclusiveBitMask](const Value curValue) { + // Set exclusive flag and make the ref count non-zero (to distinguish + // from exclusive case). This extra ref will not be reported to the + // user + return (curValue + static_cast(1)) | exclusiveBitMask; + }; + + return atomicUpdateValue(predicate, newValue); + } + + Value unmarkMoving() noexcept { + XDCHECK(isMoving()); + auto predicate = [](const Value curValue) { + XDCHECK((curValue & kAccessRefMask) != 0); + return true; + }; + + Value retValue; + auto newValue = [&retValue](const Value curValue) { + retValue = + (curValue - static_cast(1)) & ~getAdminRef(); + return retValue; + }; + + auto updated = atomicUpdateValue(predicate, newValue); + XDCHECK(updated); + + return retValue & kRefMask; + } + + bool isMoving() const noexcept { + auto raw = getRaw(); + return (raw & getAdminRef()) && ((raw & kAccessRefMask) != 0); + } + + /** + * This function attempts to mark item for eviction. + * Can only be called on the item that is moving. + * + * Returns true and marks the item for eviction only if item isOnlyMoving. + * Leaves the item marked as moving and returns false otherwise. + */ + bool markForEvictionWhenMoving() { + XDCHECK(isMoving()); + + auto predicate = [](const Value curValue) { + return (curValue & kAccessRefMask) == 1; + }; + + auto newValue = [](const Value curValue) { + XDCHECK((curValue & kAccessRefMask) == 1); + return (curValue - static_cast(1)); + }; + + return atomicUpdateValue(predicate, newValue); } - bool isOnlyExclusive() const noexcept { - // An item is only exclusive when its refcount is zero and only the - // exclusive bit among all the control bits is set. This indicates an item - // is exclusive to the current thread. No other thread is allowed to - // do anything with it. + + bool isOnlyMoving() const noexcept { + // An item is only moving when its refcount is one and only the exclusive + // bit among all the control bits is set. This indicates an item is already + // on its way out of cache. auto ref = getRefWithAccessAndAdmin(); - bool anyOtherBitSet = ref & ~getAdminRef(); - if (anyOtherBitSet) { + Value valueWithoutExclusiveBit = ref & ~getAdminRef(); + if (valueWithoutExclusiveBit != 1) { return false; } return ref & getAdminRef(); @@ -370,6 +466,39 @@ class FOLLY_PACK_ATTR RefcountWithFlags { } private: + /** + * Helper function to modify refCount_ atomically. + * + * If predicate(currentValue) is true, then it atomically assigns result + * of newValueF(currentValue) to refCount_ and returns true. Otherwise + * returns false and leaves refCount_ unmodified. + */ + template + bool atomicUpdateValue(P&& predicate, F&& newValueF) { + Value* const refPtr = &refCount_; + unsigned int nCASFailures = 0; + constexpr bool isWeak = false; + Value curValue = __atomic_load_n(refPtr, __ATOMIC_RELAXED); + while (true) { + if (!predicate(curValue)) { + return false; + } + + const Value newValue = newValueF(curValue); + if (__atomic_compare_exchange_n(refPtr, &curValue, newValue, isWeak, + __ATOMIC_ACQ_REL, __ATOMIC_RELAXED)) { + return true; + } + + if ((++nCASFailures % 4) == 0) { + // this pause takes up to 40 clock cycles on intel and the lock cmpxchgl + // above should take about 100 clock cycles. we pause once every 400 + // cycles or so if we are extremely unlucky. + folly::asm_volatile_pause(); + } + } + } + template static Value getFlag() noexcept { static_assert(flagBit >= kNumAccessRefBits + kNumAdminRefBits, diff --git a/cachelib/allocator/tests/ItemTest.cpp b/cachelib/allocator/tests/ItemTest.cpp index b0f3a2fdec..70dd1277fe 100644 --- a/cachelib/allocator/tests/ItemTest.cpp +++ b/cachelib/allocator/tests/ItemTest.cpp @@ -83,10 +83,20 @@ TEST(ItemTest, ExpiryTime) { EXPECT_EQ(tenMins, item->getConfiguredTTL()); // Test that writes fail while the item is moving - item->markExclusive(); + result = item->markMoving(); + EXPECT_TRUE(result); + result = item->updateExpiryTime(0); + EXPECT_FALSE(result); + item->unmarkMoving(); + + // Test that writes fail while the item is marked for eviction + item->markAccessible(); + result = item->markForEviction(); + EXPECT_TRUE(result); result = item->updateExpiryTime(0); EXPECT_FALSE(result); - item->unmarkExclusive(); + item->unmarkForEviction(); + item->unmarkAccessible(); // Test that writes fail while the item is not in an MMContainer item->unmarkInMMContainer(); diff --git a/cachelib/allocator/tests/RefCountTest.cpp b/cachelib/allocator/tests/RefCountTest.cpp index b355a48a8e..1f31894ddc 100644 --- a/cachelib/allocator/tests/RefCountTest.cpp +++ b/cachelib/allocator/tests/RefCountTest.cpp @@ -30,6 +30,7 @@ class RefCountTest : public AllocTestBase { public: static void testMultiThreaded(); static void testBasic(); + static void testMarkForEvictionAndMoving(); }; void RefCountTest::testMultiThreaded() { @@ -81,7 +82,7 @@ void RefCountTest::testBasic() { ASSERT_EQ(0, ref.getRaw()); ASSERT_FALSE(ref.isInMMContainer()); ASSERT_FALSE(ref.isAccessible()); - ASSERT_FALSE(ref.isExclusive()); + ASSERT_FALSE(ref.isMoving()); ASSERT_FALSE(ref.template isFlagSet()); ASSERT_FALSE(ref.template isFlagSet()); @@ -89,7 +90,7 @@ void RefCountTest::testBasic() { ref.markInMMContainer(); ASSERT_TRUE(ref.isInMMContainer()); ASSERT_FALSE(ref.isAccessible()); - ASSERT_FALSE(ref.isExclusive()); + ASSERT_FALSE(ref.isMoving()); ASSERT_EQ(0, ref.getAccessRef()); ASSERT_FALSE(ref.template isFlagSet()); ASSERT_FALSE(ref.template isFlagSet()); @@ -105,13 +106,13 @@ void RefCountTest::testBasic() { // Incrementing past the max will fail auto rawRef = ref.getRaw(); - ASSERT_FALSE(ref.incRef()); + ASSERT_THROW(ref.incRef(), std::overflow_error); ASSERT_EQ(rawRef, ref.getRaw()); // Bumping up access ref shouldn't affect admin ref and flags ASSERT_TRUE(ref.isInMMContainer()); ASSERT_FALSE(ref.isAccessible()); - ASSERT_FALSE(ref.isExclusive()); + ASSERT_FALSE(ref.isMoving()); ASSERT_EQ(RefcountWithFlags::kAccessRefMask, ref.getAccessRef()); ASSERT_TRUE(ref.template isFlagSet()); ASSERT_FALSE(ref.template isFlagSet()); @@ -128,7 +129,7 @@ void RefCountTest::testBasic() { // Bumping down access ref shouldn't affect admin ref and flags ASSERT_TRUE(ref.isInMMContainer()); ASSERT_FALSE(ref.isAccessible()); - ASSERT_FALSE(ref.isExclusive()); + ASSERT_FALSE(ref.isMoving()); ASSERT_EQ(0, ref.getAccessRef()); ASSERT_TRUE(ref.template isFlagSet()); ASSERT_FALSE(ref.template isFlagSet()); @@ -136,7 +137,7 @@ void RefCountTest::testBasic() { ref.template unSetFlag(); ASSERT_TRUE(ref.isInMMContainer()); ASSERT_FALSE(ref.isAccessible()); - ASSERT_FALSE(ref.isExclusive()); + ASSERT_FALSE(ref.isMoving()); ASSERT_EQ(0, ref.getAccessRef()); ASSERT_FALSE(ref.template isFlagSet()); ASSERT_FALSE(ref.template isFlagSet()); @@ -145,33 +146,104 @@ void RefCountTest::testBasic() { ASSERT_EQ(0, ref.getRaw()); ASSERT_FALSE(ref.isInMMContainer()); ASSERT_FALSE(ref.isAccessible()); - ASSERT_FALSE(ref.isExclusive()); + ASSERT_FALSE(ref.isMoving()); ASSERT_EQ(0, ref.getAccessRef()); ASSERT_FALSE(ref.template isFlagSet()); ASSERT_FALSE(ref.template isFlagSet()); // conditionally set flags - ASSERT_FALSE((ref.markExclusive())); + ASSERT_FALSE((ref.markMoving())); ref.markInMMContainer(); - ASSERT_TRUE((ref.markExclusive())); - ASSERT_FALSE((ref.isOnlyExclusive())); + // only first one succeeds + ASSERT_TRUE((ref.markMoving())); + ASSERT_FALSE((ref.markMoving())); ref.unmarkInMMContainer(); + ref.template setFlag(); - // Have no other admin refcount but with a flag still means "isOnlyExclusive" - ASSERT_TRUE((ref.isOnlyExclusive())); + // Have no other admin refcount but with a flag still means "isOnlyMoving" + ASSERT_TRUE((ref.isOnlyMoving())); - // Set some flags and verify that "isOnlyExclusive" does not care about flags + // Set some flags and verify that "isOnlyMoving" does not care about flags ref.markIsChainedItem(); ASSERT_TRUE(ref.isChainedItem()); - ASSERT_TRUE((ref.isOnlyExclusive())); + ASSERT_TRUE((ref.isOnlyMoving())); ref.unmarkIsChainedItem(); ASSERT_FALSE(ref.isChainedItem()); - ASSERT_TRUE((ref.isOnlyExclusive())); + ASSERT_TRUE((ref.isOnlyMoving())); +} + +void RefCountTest::testMarkForEvictionAndMoving() { + { + // cannot mark for eviction when not in MMContainer + RefcountWithFlags ref; + ASSERT_FALSE(ref.markForEviction()); + } + + { + // can mark for eviction when in MMContainer + // and unmarkForEviction return value contains admin bits + RefcountWithFlags ref; + ref.markInMMContainer(); + ASSERT_TRUE(ref.markForEviction()); + ASSERT_TRUE(ref.unmarkForEviction() > 0); + } + + { + // cannot mark for eviction when moving + RefcountWithFlags ref; + ref.markInMMContainer(); + + ASSERT_TRUE(ref.markMoving()); + ASSERT_FALSE(ref.markForEviction()); + + ref.unmarkInMMContainer(); + auto ret = ref.unmarkMoving(); + ASSERT_EQ(ret, 0); + } + + { + // cannot mark moving when marked for eviction + RefcountWithFlags ref; + ref.markInMMContainer(); + + ASSERT_TRUE(ref.markForEviction()); + ASSERT_FALSE(ref.markMoving()); + + ref.unmarkInMMContainer(); + auto ret = ref.unmarkForEviction(); + ASSERT_EQ(ret, 0); + } + + { + // can mark moving when ref count > 0 + RefcountWithFlags ref; + ref.markInMMContainer(); + + ref.incRef(); + + ASSERT_TRUE(ref.markMoving()); + + ref.unmarkInMMContainer(); + auto ret = ref.unmarkMoving(); + ASSERT_EQ(ret, 1); + } + + { + // cannot mark for eviction when ref count > 0 + RefcountWithFlags ref; + ref.markInMMContainer(); + + ref.incRef(); + ASSERT_FALSE(ref.markForEviction()); + } } } // namespace TEST_F(RefCountTest, MutliThreaded) { testMultiThreaded(); } TEST_F(RefCountTest, Basic) { testBasic(); } +TEST_F(RefCountTest, MarkForEvictionAndMoving) { + testMarkForEvictionAndMoving(); +} } // namespace tests } // namespace cachelib } // namespace facebook