[draft] initial sync for chained items based on head handle #75

byrnedj · 2023-04-03T20:11:08Z

This change is

igchor · 2023-04-03T22:04:26Z

cachelib/allocator/CacheAllocator-inl.h

+        Item* candidate_;
+        if (!lastTier && toRecycle_->isChainedItem()) {
+            const auto parentKey = toRecycleParent_->getKey();
+            auto lock = chainedItemLocks_.tryLockExclusive(parentKey);


Could you put a comment explaining why we need to take this lock here? Do I remember correctly that this is to always lock in the same order and avoid deadlock?

igchor · 2023-04-03T22:06:45Z

cachelib/allocator/CacheAllocator-inl.h

@@ -1612,23 +1616,31 @@ CacheAllocator<CacheTrait>::findEviction(TierId tid, PoolId pid, ClassId cid) {
        if (lastTier && shouldWriteToNvmCache(*candidate_) && !token.isValid()) {
          stats_.evictFailConcurrentFill.inc();
        } else if ( (lastTier && candidate_->markForEviction()) ||
-                    (!lastTier && candidate_->markMoving(true)) ) {
+                    (!lastTier && candidate_->markMoving()) ) {


Do we need to mark the candidate as moving if we have parent marked as moving?

I think that if we mark the parent as moving and then, verify that the parent hasn't changed we don't need to synchronize on the child item.

igchor · 2023-04-03T22:07:44Z

cachelib/allocator/CacheAllocator-inl.h

          return;
+        } else if (!lastTier && toRecycle_->isChainedItem()) {
+          XDCHECK(toRecycleParent_->isMoving());
+          toRecycleParent_->unmarkMoving();


Should we check whether return value is 0 and do cleanup here?

igchor · 2023-04-03T22:08:01Z

cachelib/allocator/CacheAllocator-inl.h

@@ -1697,6 +1720,14 @@ CacheAllocator<CacheTrait>::findEviction(TierId tid, PoolId pid, ClassId cid) {

      (*stats_.numWritebacks)[tid][pid][cid].inc();
      wakeUpWaiters(*candidate, std::move(evictedToNext));
+      if (toRecycleParent) {
+          XDCHECK(toRecycleParent->isMoving());
+          toRecycleParent->unmarkMoving();


igchor · 2023-04-13T20:23:27Z

cachelib/allocator/CacheAllocator-inl.h

@@ -862,7 +941,7 @@ CacheAllocator<CacheTrait>::releaseBackToAllocator(Item& it,
  // memory for a chained item but has decided not to insert the chained item
  // to a parent item and instead drop the chained item handle. In this case,
  // we free the chained item directly without calling remove callback.
-  if (it.isChainedItem()) {
+  if (it.isChainedItem() && &it != toRecycle) {


Why is this change needed?

igchor · 2023-04-13T20:36:36Z

cachelib/allocator/CacheAllocator-inl.h

-    if (item->markMoving(failIfRefNotZero)) {
-      markedMoving = true;
+    if (item->isChainedItem()) {
+      markedMoving = item->asChainedItem().getParentItem(compressor_)


Can we actually skip marking the actual item (only mark the parent)? The marking here is done to avoid freeing the item in releaseBackToAllocator. Is it handled differently now?

igchor · 2023-04-13T20:37:17Z

cachelib/allocator/CacheAllocator-inl.h


      evicted = &expectedParent;
      token = createPutToken(*evicted);
      if (evicted->markForEviction()) {
-        // unmark the child so it will be freed
-        item.unmarkMoving();
+        if (item.isMoving()) {


Why do we need this check now?

igchor · 2023-04-13T20:41:34Z

cachelib/allocator/CacheAllocator-inl.h

+        //XDCHECK_EQ( &candidate->asChainedItem().getParentItem(compressor_),
+        //            toRecycleParent );
+        XDCHECK(toRecycleParent->isMoving());
+        toRecycle = candidate; //we really want to recycle the child


Isn't toRecycle already equal to candidate?

igchor · 2023-04-13T20:45:18Z

cachelib/allocator/CacheAllocator-inl.h

+          auto parentHandle = acquire(toRecycleParent);
+          if (parentHandle) {
+            wakeUpWaiters(*toRecycleParent,std::move(parentHandle));
+          } else if (UNLIKELY(ref == 0)) {


Is it possible to have ``parentHandle == NULLandref != 0`? I mean, is there any `else` we need to handle? If no, perhaps let's add an assert?

igchor · 2023-04-13T20:47:43Z

cachelib/allocator/CacheAllocator-inl.h

+
+  auto& expectedParent = oldItem.getParentItem(compressor_);
+  const auto parentKey = expectedParent.getKey();
+  auto l = chainedItemLocks_.tryLockExclusive(parentKey);


Do we still need chaineItemLocks_ if we use moving bit for synchronization?

If I remember correctly, we still want to use chainedItemLocks_ to allow modifying the chain even if there is someone holding Handle to the parent item.

Is it still true for this implementation?

igchor · 2023-04-13T20:48:38Z

cachelib/allocator/Refcount.h

        return false;
      }
-      if (!flagSet || alreadyExclusive) {
+      // when we mark a chained item as moving it - it may not


When it will not be in the mmContainer?

igchor · 2023-04-13T21:18:36Z

cachelib/allocator/CacheAllocator-inl.h

@@ -3086,8 +3287,10 @@ bool CacheAllocator<CacheTrait>::moveForSlabRelease(
       ++itemMovingAttempts) {
    stats_.numMoveAttempts.inc();

-    // Nothing to move and the key is likely also bogus for chained items.
+    // Nothing to move - in the case that tryMoving failed


Would it be possible to add support for SlabRelease in a separate PR? I think if we leave the current logic (just return false for moveForSlabRelease for chained item, everything should work fine (chained items will always be evicted).

It would make reviewing (and testing) simpler.

igchor · 2023-04-13T21:42:58Z

@byrnedj Can you also write down why we want to synchronize on either parent or head item (I forgot)? Why not just synchronize on the child? I know that this changes the semantics a bit, but I'm just wondering if there are any fundamental reasons we can't do that.

As I'm thinking about this right now, it seems to me that we should be able to make it work. One problem with such approach would be evicting the chain if moving fails (because we cannot use markForEviction when moving, we need to try to mark the parent for eviction). But we can just unmark the child, try to mark the parent and loop again if that fails.

This way, the only thing we would need to do on read path is to modify each loop that iterates over the chain to obtain handle for each child / manually check isMoving(). We could even probably just reuse moveChainedItem if we'd generalize the code for replacingInMMContainer. Something like this: igchor@f62b7af (does not compile yet)

We would also need to skip creating the handles for child items in the thread that actually moves the item between chains (otherwise we would have a deadlock - that thread would wait on item that it itself marked as moving) but it also seems doable.

vinser52 · 2023-04-14T15:41:46Z

cachelib/allocator/Refcount.h

+      if (isChained && (curValue & kAccessRefMask) > 1) {
+        return false;
+      } else if (!isChained && (curValue & kAccessRefMask) != 0) {
        return false;
      }


I think it could be simplified to be something like this::

if ((curValue & kAccessRefMask) != isChained ? 1 : 0) { return false; }

igchor · 2023-04-21T17:41:29Z

cachelib/allocator/CacheAllocator-inl.h

-        item.unmarkMoving();
+        // TODO entire chain just gets evicted since moveForSlabRelease
+        // returns false
+        XDCHECK(!item.isMoving());


Why this assert? Can't item still be moving in this case? E.g. due to markForSlabRelease

igchor · 2023-04-21T17:56:08Z

cachelib/allocator/CacheAllocator-inl.h

@@ -1697,12 +1533,25 @@ CacheAllocator<CacheTrait>::findEviction(TierId tid, PoolId pid, ClassId cid) {
            chainedItem_
                ? &toRecycle_->asChainedItem().getParentItem(compressor_)
                : nullptr;
+        auto l_ = chainedItem_ 


I don;t think we should take a lock without checking if the item is chained first - it will have significant overhead I think

item for moving. Do not support slab release logic yet.

…cRef and markMoving

byrnedj · 2023-05-26T14:53:29Z

This is implemented in #84

igchor reviewed Apr 3, 2023

View reviewed changes

byrnedj force-pushed the chained-items-v2 branch from e704d65 to 6d847fe Compare April 13, 2023 18:44

igchor reviewed Apr 13, 2023

View reviewed changes

byrnedj force-pushed the chained-items-v2 branch from e775da1 to 228a3e2 Compare April 14, 2023 13:37

vinser52 reviewed Apr 14, 2023

View reviewed changes

igchor reviewed Apr 21, 2023

View reviewed changes

byrnedj force-pushed the chained-items-v2 branch from a6f8593 to 267f7d7 Compare April 27, 2023 14:13

igchor and others added 6 commits April 27, 2023 07:13

Chained item movement between tiers - currently sync on the parent

5733d49

item for moving. Do not support slab release logic yet.

Remove parameter for markMoving()

7c53f57

still locked on chain item lock

b36b7ad

still races

0f7704a

Remove separate code path for single-tier and get rid of params to in…

d4fe4a8

…cRef and markMoving

updated to work with slab release process

267f7d7

byrnedj closed this May 26, 2023

[draft] initial sync for chained items based on head handle #75

[draft] initial sync for chained items based on head handle #75

Uh oh!

Conversation

byrnedj commented Apr 3, 2023 • edited by rdower Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igchor commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igchor Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byrnedj commented May 26, 2023

Uh oh!

Uh oh!

byrnedj commented Apr 3, 2023 •

edited by rdower

Loading

igchor commented Apr 13, 2023 •

edited

Loading

igchor Apr 21, 2023 •

edited

Loading