Skip to content

Fix race in acquire #68

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 16, 2023
Merged

Fix race in acquire #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions cachelib/allocator/CacheAllocator-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1024,16 +1024,21 @@ CacheAllocator<CacheTrait>::acquire(Item* it) {
SCOPE_FAIL { stats_.numRefcountOverflow.inc(); };

// TODO: do not block incRef for child items to avoid deadlock
auto failIfMoving = getNumTiers() > 1 && !it->isChainedItem();
auto incRes = incRef(*it, failIfMoving);
if (LIKELY(incRes == RefcountWithFlags::incResult::incOk)) {
return WriteHandle{it, *this};
} else if (incRes == RefcountWithFlags::incResult::incFailedEviction){
// item is being evicted
return WriteHandle{};
} else {
// item is being moved - wait for completion
return handleWithWaitContextForMovingItem(*it);
const auto failIfMoving = getNumTiers() > 1 && !it->isChainedItem();

while (true) {
Copy link

@vinser52 vinser52 Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inside this spin loop, it is a good practice to use pause instructions. Actually, folly already has platform-agnostic function asm_volatile_pause(). Furthermore, there is a folly::detail::Sleeper abstraction, but the problem is that it is not a public interface.

Copy link
Author

@igchor igchor Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this loop is actually guaranteed to have at most 2 iterations. I'll try to verify this experimentally but here's my thinking:

acquire() is always called under Access Container lock so there is only one scenario (1.a.) when the race can happen on the evicting 'item':

  1. tryEvictToNextMemoryTier failed, or we are evicting from the last tier
    a. markForEvictionWhenMoving is called just after we got incFailedMoving: if under the MoveLock we realize that item is no longer moving, it has has to be marked for eviction so we just return NULL (we loop once and fail with incFailedEviction). There's no use-after free on the item because acquire holds AC lock, so findEviction will block on unlinkItemForEviction.
    b. in all other cases we synchronize on AC lock or the item is already marked for eviction (and we just return NULL).
  2. the item is successfully moved between tiers
    a. unmarMoving is called just after we got incFailedMoving? This cannot happen because unmarkMoving is called AFTER AC->replaceIf (which takes AC lock) which means we wouldn't enter acquire with pointer to the item that is being evicted.

We also need to consider newItemHdl. If find or inserOrReplace gets newItem that is still marked moving (replaceIf in moveRegularItemWithSync was executed but unmarkMoving was not) and the newItem is no longer moving under MoveLock then we just increment the item (it will work since it's no longer moving).

If someone will try to evict/move this new item before acquire gets a chance to incRef successfully, acquire will surely succeed in creating a wait context (because acquire() is under AC lock so findEviction cannot replaceIf or remove from access container which is always done before unmarking).

auto incRes = incRef(*it, failIfMoving);
if (LIKELY(incRes == RefcountWithFlags::incResult::incOk)) {
return WriteHandle{it, *this};
} else if (incRes == RefcountWithFlags::incResult::incFailedEviction){
// item is being evicted
return WriteHandle{};
} else {
// item is being moved - wait for completion
WriteHandle handle;
if (tryGetHandleWithWaitContextForMovingItem(*it, handle))
return handle;
}
}
}

Expand Down Expand Up @@ -1255,20 +1260,25 @@ CacheAllocator<CacheTrait>::insertOrReplace(const WriteHandle& handle) {
* 3. Wait until the moving thread will complete its job.
*/
template <typename CacheTrait>
typename CacheAllocator<CacheTrait>::WriteHandle
CacheAllocator<CacheTrait>::handleWithWaitContextForMovingItem(Item& item) {
bool
CacheAllocator<CacheTrait>::tryGetHandleWithWaitContextForMovingItem(Item& item, WriteHandle& handle) {
auto shard = getShardForKey(item.getKey());
auto& movesMap = getMoveMapForShard(shard);
{
auto lock = getMoveLockForShard(shard);

// item might have been evicted or moved before the lock was acquired
if (!item.isMoving())
return false;

WriteHandle hdl{*this};
auto waitContext = hdl.getItemWaitContext();

auto ret = movesMap.try_emplace(item.getKey(), std::make_unique<MoveCtx>());
ret.first->second->addWaiter(std::move(waitContext));

return hdl;
handle = std::move(hdl);
return true;
}
}

Expand Down
2 changes: 1 addition & 1 deletion cachelib/allocator/CacheAllocator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2250,7 +2250,7 @@ auto& mmContainer = getMMContainer(tid, pid, cid);

size_t memoryTierSize(TierId tid) const;

WriteHandle handleWithWaitContextForMovingItem(Item& item);
bool tryGetHandleWithWaitContextForMovingItem(Item& item, WriteHandle& handle);

size_t wakeUpWaitersLocked(folly::StringPiece key, WriteHandle&& handle);

Expand Down