Chained item movement between tiers - sync on the parent item (#84)

* Chained item movement between tiers - currently sync on the parent
item for moving.
 - updated tests accordingly, note that we can no longer swap
   parent item if chained item is being moved for slab release.

* added some debug checks around chained item check

---------

Co-authored-by: Igor Chorazewicz <[email protected]>

Author: byrnedj

cachelib/allocator/CacheAllocator-inl.h

@@ -1387,7 +1387,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
@@ -1551,6 +1551,26 @@ 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 yet
+  // and if we fail to link to the chain for any reasoin
+  // the chained item 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.
   //
@@ -1626,19 +1646,17 @@ class CacheAllocator : public CacheBase {
   //
   // @return true  If the move was completed, and the containers were updated
   //               successfully.
-  bool moveRegularItemWithSync(Item& oldItem, WriteHandle& newItemHdl);
+  bool moveRegularItem(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 moveChainedItem(ChainedItem& oldItem, WriteHandle& newItemHdl, Item& parentItem);
 
   // template class for viewAsChainedAllocs that takes either ReadHandle or
   // WriteHandle
@@ -1651,29 +1669,12 @@ class CacheAllocator : public CacheBase {
   template <typename Handle>
   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.
@@ -1682,7 +1683,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
@@ -1696,6 +1697,24 @@ class CacheAllocator : public CacheBase {
                                        WriteHandle newItemHdl,
                                        const Item& parent);
 
+  //
+  // Performs the actual inplace replace - it is called from
+  // moveChainedItem 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
+  // @param fromMove used to determine if the replaced was called from
+  //                 moveChainedItem - we avoid the handle destructor
+  //                 in this case.
+  //
+  // @return handle to the oldItem
+  void replaceInChainLocked(Item& oldItem,
+                            WriteHandle& newItemHdl,
+                            const Item& parent,
+                            bool fromMove);
+
   // Insert an item into MM container. The caller must hold a valid handle for
   // the item.
   //
@@ -1986,7 +2005,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 {
@@ -2059,7 +2078,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);

cachelib/allocator/CacheAllocator.h

@@ -238,8 +238,8 @@ bool CacheItem<CacheTrait>::markForEvictionWhenMoving() {
 }
 
 template <typename CacheTrait>
-bool CacheItem<CacheTrait>::markMoving(bool failIfRefNotZero) {
-  return ref_.markMoving(failIfRefNotZero);
+bool CacheItem<CacheTrait>::markMoving() {
+  return ref_.markMoving();
 }
 
 template <typename CacheTrait>

cachelib/allocator/CacheItem-inl.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;

cachelib/allocator/CacheItem.h

@@ -247,6 +247,12 @@ MM2Q::Container<T, HookPtr>::getEvictionIterator() const noexcept {
   return LockedIterator{std::move(l), lru_.rbegin()};
 }
 
+template <typename T, MM2Q::Hook<T> T::*HookPtr>
+template <typename F>
+void MM2Q::Container<T, HookPtr>::withContainerLock(F&& fun) {
+  lruMutex_->lock_combine([this, &fun]() { fun(); });
+}
+
 template <typename T, MM2Q::Hook<T> T::*HookPtr>
 template <typename F>
 void MM2Q::Container<T, HookPtr>::withEvictionIterator(F&& fun) {

cachelib/allocator/MM2Q-inl.h

@@ -503,6 +503,10 @@ class MM2Q {
     template <typename F>
     void withEvictionIterator(F&& f);
 
+    // Execute provided function under container lock.
+    template <typename F>
+    void withContainerLock(F&& f);
+
     // Execute provided function under container lock. Function gets
     // iterator passed as parameter.
     template <typename F>

cachelib/allocator/MM2Q.h

@@ -218,6 +218,12 @@ MMLru::Container<T, HookPtr>::getEvictionIterator() const noexcept {
   return LockedIterator{std::move(l), lru_.rbegin()};
 }
 
+template <typename T, MMLru::Hook<T> T::*HookPtr>
+template <typename F>
+void MMLru::Container<T, HookPtr>::withContainerLock(F&& fun) {
+  lruMutex_->lock_combine([this, &fun]() { fun(); });
+}
+
 template <typename T, MMLru::Hook<T> T::*HookPtr>
 template <typename F>
 void MMLru::Container<T, HookPtr>::withEvictionIterator(F&& fun) {

cachelib/allocator/MMLru-inl.h

@@ -377,6 +377,10 @@ class MMLru {
     template <typename F>
     void withEvictionIterator(F&& f);
 
+    // Execute provided function under container lock.
+    template <typename F>
+    void withContainerLock(F&& f);
+
     template <typename F>
     void withPromotionIterator(F&& f);
 

cachelib/allocator/MMLru.h

@@ -220,6 +220,13 @@ MMTinyLFU::Container<T, HookPtr>::getEvictionIterator() const noexcept {
   return LockedIterator{std::move(l), *this};
 }
 
+template <typename T, MMTinyLFU::Hook<T> T::*HookPtr>
+template <typename F>
+void MMTinyLFU::Container<T, HookPtr>::withContainerLock(F&& fun) {
+  LockHolder l(lruMutex_);
+  fun();
+}
+
 template <typename T, MMTinyLFU::Hook<T> T::*HookPtr>
 template <typename F>
 void MMTinyLFU::Container<T, HookPtr>::withEvictionIterator(F&& fun) {

cachelib/allocator/MMTinyLFU-inl.h

@@ -497,6 +497,10 @@ class MMTinyLFU {
     template <typename F>
     void withEvictionIterator(F&& f);
 
+    // Execute provided function under container lock.
+    template <typename F>
+    void withContainerLock(F&& f);
+
     template <typename F>
     void withPromotionIterator(F&& f);
 

cachelib/allocator/MMTinyLFU.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<kExclusive>();
 
        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,15 @@ 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<kLinked>();
       const bool flagSet = curValue & conditionBitMask;
       const bool alreadyExclusive = curValue & getAdminRef<kExclusive>();
-      if (failIfRefNotZero && (curValue & kAccessRefMask) != 0) {
+      const bool isChained = curValue & getFlag<kIsChainedItem>();
+
+      // chained item can have ref count == 1, this just means it's linked in the chain
+      if ((curValue & kAccessRefMask) > isChained ? 1 : 0) {
         return false;
       }
       if (!flagSet || alreadyExclusive) {

cachelib/allocator/Refcount.h

@@ -288,8 +288,8 @@ TYPED_TEST(BaseAllocatorTest, AddChainedItemMultiThreadWithMovingAndSync) {
   this->testAddChainedItemMultithreadWithMovingAndSync();
 }
 
-TYPED_TEST(BaseAllocatorTest, TransferChainWhileMoving) {
-  this->testTransferChainWhileMoving();
+TYPED_TEST(BaseAllocatorTest, TransferChainAfterMoving) {
+  this->testTransferChainAfterMoving();
 }
 
 TYPED_TEST(BaseAllocatorTest, AddAndPopChainedItemMultithread) {