From b6e33dbc0e9787213b9a0ca8f232592ded68a30f Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Fri, 21 Oct 2022 12:09:03 -0700 Subject: [PATCH] Fix HyperClockCache Rollback bug in #10801 (#10843) Summary: In https://github.com/facebook/rocksdb/issues/10801 in ClockHandleTable::Evict, we saved a reference to the hash value (`const UniqueId64x2& hashed_key`) instead of saving the hash value itself before marking the handle as empty and thus free for use by other threads. This could lead to Rollback seeing the wrong hash value for updating the `displacements` after an entry is removed. The fix is (like other places) to copy the hash value before it's released. (We could Rollback while we own the entry, but that creates more dependences between atomic updates, because in that case, based on the code, the Rollback writes would have to happen before or after the entry is released by marking empty. By doing the relaxed Rollback after marking empty, there's more opportunity for re-ordering / ILP.) Intended follow-up: refactoring for better code sharing in clock_cache.cc Pull Request resolved: https://github.com/facebook/rocksdb/pull/10843 Test Plan: watch for clean crash test, TSAN Reviewed By: siying Differential Revision: D40579680 Pulled By: pdillinger fbshipit-source-id: 258e43b3b80bc980a161d5c675ccc6708ecb8025 --- cache/clock_cache.cc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/cache/clock_cache.cc b/cache/clock_cache.cc index d353c9966..27793de9c 100644 --- a/cache/clock_cache.cc +++ b/cache/clock_cache.cc @@ -919,11 +919,13 @@ void ClockHandleTable::Evict(size_t requested_charge, size_t* freed_charge, uint64_t{ClockHandle::kStateConstruction} << ClockHandle::kStateShift, std::memory_order_acquire)) { - // Took ownership - const UniqueId64x2& hashed_key = h.hashed_key; + // Took ownership. + // Save info about h to minimize dependences between atomic updates + // (e.g. fully relaxed Rollback after h released by marking empty) + const UniqueId64x2 h_hashed_key = h.hashed_key; + size_t h_total_charge = h.total_charge; // TODO? Delay freeing? h.FreeData(); - *freed_charge += h.total_charge; #ifndef NDEBUG // Mark slot as empty, with assertion meta = h.meta.exchange(0, std::memory_order_release); @@ -934,7 +936,8 @@ void ClockHandleTable::Evict(size_t requested_charge, size_t* freed_charge, h.meta.store(0, std::memory_order_release); #endif *freed_count += 1; - Rollback(hashed_key, &h); + *freed_charge += h_total_charge; + Rollback(h_hashed_key, &h); } }