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
main
Peter Dillinger 2 years ago committed by Facebook GitHub Bot
parent 333abe9c55
commit b6e33dbc0e
  1. 11
      cache/clock_cache.cc

@ -919,11 +919,13 @@ void ClockHandleTable::Evict(size_t requested_charge, size_t* freed_charge,
uint64_t{ClockHandle::kStateConstruction} uint64_t{ClockHandle::kStateConstruction}
<< ClockHandle::kStateShift, << ClockHandle::kStateShift,
std::memory_order_acquire)) { std::memory_order_acquire)) {
// Took ownership // Took ownership.
const UniqueId64x2& hashed_key = h.hashed_key; // 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? // TODO? Delay freeing?
h.FreeData(); h.FreeData();
*freed_charge += h.total_charge;
#ifndef NDEBUG #ifndef NDEBUG
// Mark slot as empty, with assertion // Mark slot as empty, with assertion
meta = h.meta.exchange(0, std::memory_order_release); 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); h.meta.store(0, std::memory_order_release);
#endif #endif
*freed_count += 1; *freed_count += 1;
Rollback(hashed_key, &h); *freed_charge += h_total_charge;
Rollback(h_hashed_key, &h);
} }
} }

Loading…
Cancel
Save