enable combined locking with exclusive bit

Author: byrnedj

cachelib/allocator/CacheAllocator-inl.h

@@ -1389,7 +1389,7 @@ class CacheAllocator : public CacheBase {
   double slabsApproxFreePercentage(TierId tid) const;
 
   // 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
@@ -1440,6 +1440,12 @@ class CacheAllocator : public CacheBase {
                                     bool nascent = false,
                                     const Item* toRecycle = nullptr);
 
+  // Must be called by the thread which called markExclusive and
+  // succeeded. After this call, the item is unlinked from Access and
+  // MM Containers. The item is no longer marked as exclusive and it's
+  // ref count is 0 - it's available for recycling.
+  void unlinkItemExclusive(Item& it);
+
   // acquires an handle on the item. returns an empty handle if it is null.
   // @param it    pointer to an item
   // @return WriteHandle   return a handle to this item
@@ -1550,17 +1556,17 @@ class CacheAllocator : public CacheBase {
   // @return  handle to the parent item if the validations pass
   //          otherwise, an empty Handle is returned.
   //
-  ReadHandle validateAndGetParentHandleForChainedMoveLocked(
+  WriteHandle validateAndGetParentHandleForChainedMoveLocked(
       const ChainedItem& item, const Key& parentKey);
 
   // Given an existing item, allocate a new one for the
   // existing one to later be moved into.
   //
-  // @param oldItem    the item we want to allocate a new item for
+  // @param item   reference to the item we want to allocate a new item for
   //
   // @return  handle to the newly allocated item
   //
-  WriteHandle allocateNewItemForOldItem(const Item& oldItem);
+  WriteHandle allocateNewItemForOldItem(const Item& item);
 
   // internal helper that grabs a refcounted handle to the item. This does
   // not record the access to reflect in the mmContainer.
@@ -1614,18 +1620,17 @@ class CacheAllocator : public CacheBase {
   // @param oldItem     Reference to the item being moved
   // @param newItemHdl  Reference to the handle of the new item being moved into
   //
-  // @return            the handle to the oldItem if the move was completed
-  //                    and the oldItem can be recycled.
-  //                    Otherwise an empty handle is returned.
+  // @return true  If the move was completed, and the containers were updated
+  //               successfully.
   template <typename P>
-  WriteHandle moveRegularItemWithSync(Item& oldItem, WriteHandle& newItemHdl, P&& predicate);
+  bool moveRegularItemWithSync(Item& oldItem, WriteHandle& newItemHdl, P&& predicate);
 
   // 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.
   //
-  // @param oldItem     Reference to the item being moved
+  // @param oldItem     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
@@ -1787,7 +1792,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.
-  WriteHandle tryEvictToNextMemoryTier(TierId tid, PoolId pid, Item& item, bool fromBgThread);
+  bool tryEvictToNextMemoryTier(TierId tid, PoolId pid, Item& item, bool fromBgThread);
 
   bool tryPromoteToNextMemoryTier(TierId tid, PoolId pid, Item& item, bool fromBgThread);
 
@@ -1799,7 +1804,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. 
-  WriteHandle tryEvictToNextMemoryTier(Item& item, bool fromBgThread);
+  bool tryEvictToNextMemoryTier(Item& item, bool fromBgThread);
 
   size_t memoryTierSize(TierId tid) const;
 
@@ -1878,22 +1883,23 @@ 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.
   //
   //
   // @param ctx         slab release context
-  // @param item        old item to be moved elsewhere
+  // @param oldItem     old item to be moved elsewhere
+  // @param handle      handle to the item or to it's parent (if chained)
   // @param throttler   slow this function down as not to take too much cpu
   //
   // @return    true  if the item has been moved
   //            false if we have exhausted moving attempts
   bool moveForSlabRelease(const SlabReleaseContext& ctx,
-                          Item& item,
+                          Item& oldItem,
                           util::Throttler& throttler);
 
   // "Move" (by copying) the content in this item to another memory
@@ -1929,6 +1935,8 @@ class CacheAllocator : public CacheBase {
   // handle on failure. caller can retry.
   WriteHandle evictChainedItemForSlabRelease(ChainedItem& item);
 
+  typename NvmCacheT::PutToken createPutToken(Item& item);
+
   // Helper function to remove a item if predicates is true.
   //
   // @return last handle to the item on success. empty handle on failure.
@@ -1966,8 +1974,10 @@ class CacheAllocator : public CacheBase {
     candidates.reserve(batch);
 
     size_t tries = 0;
-    mmContainer.withEvictionIterator([&tries, &candidates, &batch, this](auto &&itr){
-      while (candidates.size() < batch && (config_.maxEvictionPromotionHotness == 0 || tries < config_.maxEvictionPromotionHotness) && itr) {
+    mmContainer.withEvictionIterator([&tries, &candidates, &batch, &mmContainer, this](auto &&itr) {
+      while (candidates.size() < batch && 
+        (config_.maxEvictionPromotionHotness == 0 || tries < config_.maxEvictionPromotionHotness) && 
+         itr) {
         tries++;
         Item* candidate = itr.get();
         XDCHECK(candidate);
@@ -1976,7 +1986,8 @@ class CacheAllocator : public CacheBase {
           throw std::runtime_error("Not supported for chained items");
         }
 
-        if (candidate->getRefCount() == 0 && candidate->markExclusive()) {
+        if (candidate->markExclusive()) {
+          mmContainer.remove(itr);
           candidates.push_back(candidate);
         }
 
@@ -1985,37 +1996,29 @@ class CacheAllocator : public CacheBase {
     });
 
     for (Item *candidate : candidates) {
-      {
-        auto toReleaseHandle =
-          evictNormalItem(*candidate,
-			  true /* skipIfTokenInvalid */, true /* from BG thread */);
-	// destroy toReleseHandle. The item won't be release to allocator
-        // since we marked it as exclusive.
+      auto evictedToNext = tryEvictToNextMemoryTier(*candidate, true /* from BgThread */);
+      XDCHECK(evictedToNext);
+      if (evictedToNext) {
+          auto ref = candidate->unmarkExclusive();
+          XDCHECK(ref == 0u);
+          evictions++;
+      } else {
+	      unlinkItemExclusive(*candidate);
       }
-      auto ref = candidate->unmarkExclusive();
-
-      if (ref == 0u) {
-        if (candidate->hasChainedItem()) {
-          (*stats_.chainedItemEvictions)[pid][cid].inc();
-        } else {
-          (*stats_.regularItemEvictions)[pid][cid].inc();
-        }
-
-        evictions++;
-        // 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.
-        auto res = releaseBackToAllocator(*candidate, RemoveContext::kEviction,
-                                  /* isNascent */ false);
-        XDCHECK(res == ReleaseRes::kReleased);
+      XDCHECK(!candidate->isExclusive() && !candidate->isMoving());
 
+      if (candidate->hasChainedItem()) {
+        (*stats_.chainedItemEvictions)[pid][cid].inc();
       } else {
-        if (candidate->hasChainedItem()) {
-          stats_.evictFailParentAC.inc();
-        } else {
-          stats_.evictFailAC.inc();
-        }
+        (*stats_.regularItemEvictions)[pid][cid].inc();
       }
+
+      // 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.
+      auto res = releaseBackToAllocator(*candidate, RemoveContext::kEviction,
+                                /* isNascent */ false);
+      XDCHECK(res == ReleaseRes::kReleased);
     }
     return evictions;
   }
@@ -2028,7 +2031,7 @@ class CacheAllocator : public CacheBase {
 
     size_t tries = 0;
 
-    mmContainer.withPromotionIterator([&tries, &candidates, &batch, this](auto &&itr){
+    mmContainer.withPromotionIterator([&tries, &candidates, &batch, &mmContainer, this](auto &&itr){
       while (candidates.size() < batch && (config_.maxEvictionPromotionHotness == 0 || tries < config_.maxEvictionPromotionHotness) && itr) {
         tries++;
         Item* candidate = itr.get();
@@ -2038,10 +2041,10 @@ class CacheAllocator : public CacheBase {
           throw std::runtime_error("Not supported for chained items");
         }
 
-
         // TODO: only allow it for read-only items?
         // or implement mvcc
-        if (!candidate->isExpired() && candidate->markExclusive()) {
+        if (candidate->markExclusive()) {
+          mmContainer.remove(itr);
           candidates.push_back(candidate);
         }
 
@@ -2051,16 +2054,18 @@ class CacheAllocator : public CacheBase {
 
     for (Item *candidate : candidates) {
       auto promoted = tryPromoteToNextMemoryTier(*candidate, true);
-      auto ref = candidate->unmarkExclusive();
-      if (promoted)
+	  if (promoted) {
         promotions++;
-
-      if (ref == 0u) {
-        // stats_.promotionMoveSuccess.inc();
-        auto res = releaseBackToAllocator(*candidate, RemoveContext::kEviction,
-                                    /* isNascent */ false);
-        XDCHECK(res == ReleaseRes::kReleased);
       }
+      unlinkItemExclusive(*candidate);
+      XDCHECK(!candidate->isExclusive() && !candidate->isMoving());
+     
+     // 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.
+     auto res = releaseBackToAllocator(*candidate, RemoveContext::kEviction,
+                               /* isNascent */ false);
+     XDCHECK(res == ReleaseRes::kReleased);
     }
 
     return promotions;
@@ -2173,18 +2178,14 @@ class CacheAllocator : public CacheBase {
   std::optional<bool> saveNvmCache();
   void saveRamCache();
 
-  static bool itemExclusivePredicate(const Item& item) {
-    return item.getRefCount() == 0;
+  static bool itemSlabMovePredicate(const Item& item) {
+    return item.isMoving() && item.getRefCount() == 0;
   }
 
   static bool itemExpiryPredicate(const Item& item) {
     return item.getRefCount() == 1 && item.isExpired();
   }
 
-  static bool parentEvictForSlabReleasePredicate(const Item& item) {
-    return item.getRefCount() == 1 && !item.isExclusive();
-  }
-
   std::unique_ptr<Deserializer> createDeserializer();
 
   // Execute func on each item. `func` can throw exception but must ensure

cachelib/allocator/CacheAllocator.h

@@ -232,8 +232,28 @@ bool CacheItem<CacheTrait>::isExclusive() const noexcept {
 }
 
 template <typename CacheTrait>
-bool CacheItem<CacheTrait>::isOnlyExclusive() const noexcept {
-  return ref_.isOnlyExclusive();
+bool CacheItem<CacheTrait>::markExclusiveWhenMoving() {
+  return ref_.markExclusiveWhenMoving();
+}
+
+template <typename CacheTrait>
+bool CacheItem<CacheTrait>::markMoving() {
+  return ref_.markMoving();
+}
+
+template <typename CacheTrait>
+RefcountWithFlags::Value CacheItem<CacheTrait>::unmarkMoving() noexcept {
+  return ref_.unmarkMoving();
+}
+
+template <typename CacheTrait>
+bool CacheItem<CacheTrait>::isMoving() const noexcept {
+  return ref_.isMoving();
+}
+
+template <typename CacheTrait>
+bool CacheItem<CacheTrait>::isOnlyMoving() const noexcept {
+  return ref_.isOnlyMoving();
 }
 
 template <typename CacheTrait>

cachelib/allocator/CacheItem-inl.h

@@ -314,12 +314,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() {
@@ -353,23 +358,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;
+
+  /**
+   * 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 markExclusiveWhenMoving();
 
   /**
    * Item cannot be marked both chained allocation and