Combined locking with exclusive bit (#38) -

Based on cs part 2 upstream patch.

- Use markForEviction only for eviction

(findEviction and evictForSlabRelease) but not for
item movement. moveForSlabRelease relies on markMoving().

Only allow to mark item as exclusive if ref count
is 0. This ensures that after item is marked eviction
cannot fail. This makes it possible to return NULL handle
immediately from find if item is marked as exclusive.

markMoving() does have those restrictions and still allows
readers to obtain a handle to a moving item.

Also, add option to use combined locking for MMContainer
iteration.

- Block readers when item is moving

This simplifies moving code a lot.
We don't need to worry about any failure other
than allocation failure since item is protected
from any readers (and by extension, removes).

Co-authored-by: Igor Chorążewicz <igor.chorazewicz@intel.com>

Author: byrnedj

cachelib/allocator/CacheAllocator-inl.h

@@ -1321,7 +1321,7 @@ class CacheAllocator : public CacheBase {
 
  private:
   // wrapper around Item's refcount and active handle tracking
-  FOLLY_ALWAYS_INLINE bool incRef(Item& it);
+  FOLLY_ALWAYS_INLINE RefcountWithFlags::incResult incRef(Item& it, bool failIfMoving);
   FOLLY_ALWAYS_INLINE RefcountWithFlags::Value decRef(Item& it);
 
   // drops the refcount and if needed, frees the allocation back to the memory
@@ -1552,8 +1552,7 @@ class CacheAllocator : public CacheBase {
   //
   // @return true  If the move was completed, and the containers were updated
   //               successfully.
-  template <typename ItemPtr>
-  WriteHandle moveRegularItemOnEviction(ItemPtr& oldItem, WriteHandle& newItemHdl);
+  void moveRegularItemWithSync(Item& oldItem, WriteHandle& newItemHdl);
 
   // Moves a regular item to a different slab. This should only be used during
   // slab release after the item's exclusive bit has been set. The user supplied
@@ -1722,16 +1721,15 @@ 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 <typename ItemPtr>
-  WriteHandle 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
   //
   // @param item the item to evict
   //
   // @return valid handle to the item. This will be the last
   //         handle to the item. On failure an empty handle. 
-  WriteHandle tryEvictToNextMemoryTier(Item* item);
+  WriteHandle tryEvictToNextMemoryTier(Item& item);
 
   // Deserializer CacheAllocatorMetadata and verify the version
   //
@@ -1988,10 +1986,6 @@ class CacheAllocator : public CacheBase {
   static bool itemExpiryPredicate(const Item& item) {
     return item.getRefCount() == 1 && item.isExpired();
   }
-  
-  static bool itemExclusivePredicate(const Item& item) {
-    return item.getRefCount() == 0;
-  }
 
   std::unique_ptr<Deserializer> createDeserializer();
 
@@ -2044,8 +2038,9 @@ class CacheAllocator : public CacheBase {
 
   size_t memoryTierSize(TierId tid) const;
 
-  bool addWaitContextForMovingItem(
-      folly::StringPiece key, std::shared_ptr<WaitContext<ReadHandle>> waiter);
+  WriteHandle handleWithWaitContextForMovingItem(Item& item);
+
+  size_t wakeUpWaiters(folly::StringPiece key, WriteHandle&& handle);
 
   class MoveCtx {
    public:
@@ -2070,6 +2065,8 @@ class CacheAllocator : public CacheBase {
       waiters.push_back(std::move(waiter));
     }
 
+    size_t numWaiters() const { return waiters.size(); }
+
    private:
     // notify all pending waiters that are waiting for the fetch.
     void wakeUpWaiters() {

cachelib/allocator/CacheAllocator.h

@@ -238,8 +238,8 @@ bool CacheItem<CacheTrait>::markForEvictionWhenMoving() {
 }
 
 template <typename CacheTrait>
-bool CacheItem<CacheTrait>::markMoving() {
-  return ref_.markMoving();
+bool CacheItem<CacheTrait>::markMoving(bool failIfRefNotZero) {
+  return ref_.markMoving(failIfRefNotZero);
 }
 
 template <typename CacheTrait>
@@ -287,21 +287,6 @@ bool CacheItem<CacheTrait>::isNvmEvicted() const noexcept {
   return ref_.isNvmEvicted();
 }
 
-template <typename CacheTrait>
-void CacheItem<CacheTrait>::markIncomplete() noexcept {
-  ref_.markIncomplete();
-}
-
-template <typename CacheTrait>
-void CacheItem<CacheTrait>::unmarkIncomplete() noexcept {
-  ref_.unmarkIncomplete();
-}
-
-template <typename CacheTrait>
-bool CacheItem<CacheTrait>::isIncomplete() const noexcept {
-  return ref_.isIncomplete();
-}
-
 template <typename CacheTrait>
 void CacheItem<CacheTrait>::markIsChainedItem() noexcept {
   XDCHECK(!hasChainedItem());

cachelib/allocator/CacheItem-inl.h

@@ -241,14 +241,6 @@ class CACHELIB_PACKED_ATTR CacheItem {
   void unmarkNvmEvicted() noexcept;
   bool isNvmEvicted() const noexcept;
 
-  /**
-   * Marks that the item is migrating between memory tiers and
-   * not ready for access now. Accessing thread should wait.
-   */
-  void markIncomplete() noexcept;
-  void unmarkIncomplete() noexcept;
-  bool isIncomplete() const noexcept;
-
   /**
    * Function to set the timestamp for when to expire an item
    *
@@ -317,9 +309,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 bool incRef() {
+  FOLLY_ALWAYS_INLINE RefcountWithFlags::incResult incRef(bool failIfMoving) {
     try {
-      return ref_.incRef();
+      return ref_.incRef(failIfMoving);
     } catch (exception::RefcountOverflow& e) {
       throw exception::RefcountOverflow(
           folly::sformat("{} item: {}", e.what(), toString()));
@@ -386,7 +378,7 @@ class CACHELIB_PACKED_ATTR CacheItem {
    * Unmarking moving will also return the refcount at the moment of
    * unmarking.
    */
-  bool markMoving();
+  bool markMoving(bool failIfRefNotZero);
   RefcountWithFlags::Value unmarkMoving() noexcept;
   bool isMoving() const noexcept;
   bool isOnlyMoving() const noexcept;

cachelib/allocator/CacheItem.h

@@ -488,13 +488,6 @@ struct ReadHandleImpl {
   // Handle which has the item already
   FOLLY_ALWAYS_INLINE ReadHandleImpl(Item* it, CacheT& alloc) noexcept
       : alloc_(&alloc), it_(it) {
-    if (it_ && it_->isIncomplete()) {
-      waitContext_ = std::make_shared<ItemWaitContext>(alloc);
-      if (!alloc_->addWaitContextForMovingItem(it->getKey(), waitContext_)) {
-        waitContext_->discard();
-        waitContext_.reset();
-      }
-    }
   }
 
   // handle that has a wait context allocated. Used for async handles

cachelib/allocator/Handle.h

@@ -229,6 +229,18 @@ void MMLru::Container<T, HookPtr>::withEvictionIterator(F&& fun) {
   }
 }
 
+//template <typename T, MMLru::Hook<T> T::*HookPtr>
+//template <typename F>
+//void
+//MMLru::Container<T, HookPtr>::withPromotionIterator(F&& fun) {
+//  if (config_.useCombinedLockForIterators) {
+//    lruMutex_->lock_combine([this, &fun]() { fun(Iterator{lru_.begin()}); });
+//  } else {
+//    LockHolder lck{*lruMutex_};
+//    fun(Iterator{lru_.begin()});
+//  }
+//}
+
 template <typename T, MMLru::Hook<T> T::*HookPtr>
 void MMLru::Container<T, HookPtr>::ensureNotInsertionPoint(T& node) noexcept {
   // If we are removing the insertion point node, grow tail before we remove

cachelib/allocator/MMLru-inl.h

@@ -230,12 +230,13 @@ class MMLru {
     //     lruInsertionPointSpec = 2, we insert at a point 1/4th from tail
     uint8_t lruInsertionPointSpec{0};
 
+    // Whether to use combined locking for withEvictionIterator.
+    bool useCombinedLockForIterators{true};
+
     // Minimum interval between reconfigurations. If 0, reconfigure is never
     // called.
     std::chrono::seconds mmReconfigureIntervalSecs{};
 
-    // Whether to use combined locking for withEvictionIterator.
-    bool useCombinedLockForIterators{false};
   };
 
   // The container object which can be used to keep track of objects of type

cachelib/allocator/MMLru.h

@@ -116,10 +116,6 @@ class FOLLY_PACK_ATTR RefcountWithFlags {
     // unevictable in the past.
     kUnevictable_NOOP,
 
-    // Item is accecible but content is not ready yet. Used by eviction
-    // when Item is moved between memory tiers.
-    kIncomplete,
-
     // Unused. This is just to indciate the maximum number of flags
     kFlagMax,
   };
@@ -134,30 +130,47 @@ class FOLLY_PACK_ATTR RefcountWithFlags {
   RefcountWithFlags& operator=(const RefcountWithFlags&) = delete;
   RefcountWithFlags(RefcountWithFlags&&) = delete;
   RefcountWithFlags& operator=(RefcountWithFlags&&) = delete;
-
+  enum incResult {
+     incOk,
+     incFailedMoving,
+     incFailedEviction
+   };
   // Bumps up the reference count only if the new count will be strictly less
   // 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<kExclusive>();
-
-      const bool exlusiveBitIsSet = curValue & bitMask;
-      if (UNLIKELY((curValue & kAccessRefMask) == (kAccessRefMask))) {
-        throw exception::RefcountOverflow("Refcount maxed out.");
-      }
-
-      // Check if the item is not marked for eviction
-      return !exlusiveBitIsSet || ((curValue & kAccessRefMask) != 0);
-    };
-
-    auto newValue = [](const Value curValue) {
-      return (curValue + static_cast<Value>(1));
-    };
-
-    return atomicUpdateValue(predicate, newValue);
+  FOLLY_ALWAYS_INLINE incResult incRef(bool failIfMoving) {
+    auto predicate = [failIfMoving](const Value curValue) {
+       Value bitMask = getAdminRef<kExclusive>();
+
+       const bool exlusiveBitIsSet = curValue & bitMask;
+       if (UNLIKELY((curValue & kAccessRefMask) == (kAccessRefMask))) {
+         throw exception::RefcountOverflow("Refcount maxed out.");
+       } else if (exlusiveBitIsSet && (curValue & kAccessRefMask) == 0) {
+         return false;
+       } else if (exlusiveBitIsSet && failIfMoving) {
+         return false;
+       }
+       return true;
+     };
+
+     auto newValue = [](const Value curValue) {
+       return (curValue + static_cast<Value>(1));
+     };
+
+     if (atomicUpdateValue(predicate, newValue)) {
+         return incOk;
+     } 
+     Value val = __atomic_load_n(&refCount_, __ATOMIC_RELAXED);
+     Value bitMask = getAdminRef<kExclusive>();
+     const bool exlusiveBitIsSet = val & bitMask;
+     if (exlusiveBitIsSet & (val & kAccessRefMask) == 0) {
+         return incFailedEviction;
+     } else if (exlusiveBitIsSet) {
+         return incFailedMoving;
+     }
+     return incFailedEviction;
   }
 
   // Bumps down the reference count
@@ -313,12 +326,14 @@ class FOLLY_PACK_ATTR RefcountWithFlags {
    *
    * Unmarking moving does not depend on `isInMMContainer`
    */
-  bool markMoving() {
-    auto predicate = [](const Value curValue) {
+  bool markMoving(bool failIfRefNotZero) {
+    auto predicate = [failIfRefNotZero](const Value curValue) {
       Value conditionBitMask = getAdminRef<kLinked>();
       const bool flagSet = curValue & conditionBitMask;
       const bool alreadyExclusive = curValue & getAdminRef<kExclusive>();
-
+      if (failIfRefNotZero && (curValue & kAccessRefMask) != 0) {
+        return false;
+      }
       if (!flagSet || alreadyExclusive) {
         return false;
       }
@@ -419,14 +434,6 @@ class FOLLY_PACK_ATTR RefcountWithFlags {
   void unmarkNvmEvicted() noexcept { return unSetFlag<kNvmEvicted>(); }
   bool isNvmEvicted() const noexcept { return isFlagSet<kNvmEvicted>(); }
 
-  /**
-   * Marks that the item is migrating between memory tiers and
-   * not ready for access now. Accessing thread should wait.
-   */
-  void markIncomplete() noexcept { return setFlag<kIncomplete>(); }
-  void unmarkIncomplete() noexcept { return unSetFlag<kIncomplete>(); }
-  bool isIncomplete() const noexcept { return isFlagSet<kIncomplete>(); }
-
   // Whether or not an item is completely drained of access
   // Refcount is 0 and the item is not linked, accessible, nor exclusive
   bool isDrained() const noexcept { return getRefWithAccessAndAdmin() == 0; }

cachelib/allocator/Refcount.h

@@ -41,8 +41,6 @@ struct TestItem {
   void reset() {}
 
   folly::StringPiece getKey() const { return folly::StringPiece(); }
-
-  bool isIncomplete() const { return false; }
 };
 
 struct TestNvmCache;

cachelib/allocator/tests/ItemHandleTest.cpp

@@ -82,8 +82,10 @@ TEST(ItemTest, ExpiryTime) {
   EXPECT_TRUE(result);
   EXPECT_EQ(tenMins, item->getConfiguredTTL());
 
+  // So that exclusive bit will be set
+  item->markAccessible();
   // Test that writes fail while the item is moving
-  result = item->markMoving();
+  result = item->markMoving(true);
   EXPECT_TRUE(result);
   result = item->updateExpiryTime(0);
   EXPECT_FALSE(result);