fix: make FreeMemWithEvictionStep atomic #4885

kostasrim · 2025-04-03T15:58:20Z

FreeMemWithEvictionStep was not atomic yet was used by a flow -- RetireExpireAndEvict() -- which should be atomic. Therefore, we make this function atomic (since it's only used by that flow).

Resolves #4875

kostasrim · 2025-04-03T15:59:40Z

src/server/db_slice.cc

    };

    shard_set->pool()->AwaitBrief(std::move(cb));
  }
 }

+void DbSlice::SendQueuedInvalidationMessagesAsync() {


We drained it before because we blocked. Now we don't.

TODO investigate: we should call this also from heartbeat (so on the second iteration it drains it if there were items added).

After looking at this the while loop was redundant in the flow of heartbeat. Why? Because heartbeat was preempted(because it tried to sent the queued invalidated messages). pending_send_map_ can only change in two places: 1) during evictions from heartbeat 2) through db_slice and drained at the end via OnCbFinish(). The former (1) is guaranteed that it won't evict because it's preempted and until it completes it won't run another heartbeat on that thread while the later (2) drains the pending_send_map_ via OnCbFinish(). Therefore, there is no correctness issue of "missing or not draining all of the items in the pending_send_map_".

As for DispatchBrief, I also don't see any issue whatsoever, they messages will reach the connections eventually and it should be enough

DispatchBrief can also block btw, it's just a much rare event

@romange the rationale was this doesn't happen in practice because we don't reach this state easily (where we have the task queues internally full). Your comment is very valid, and maybe we should move this outside of the non preemptive critical section. I think it should be a simple change. I will update on this

@romange would it worth to add a function that won't dispatch if the task queue is full (but also won't preempt and return to the caller?)

That way, we can send the pending_items later if we can't dispatch because the intenral queues are full

Signed-off-by: kostas <[email protected]>

BorysTheDev · 2025-04-28T09:56:32Z

src/server/db_slice.cc

+        if (bucket.IsEmpty())
+          continue;
+
+        if (!bucket.IsBusy(slot_id))
+          continue;


if (bucket.IsEmpty() || !bucket.IsBusy(slot_id))
continue;

romange · 2025-04-28T14:28:10Z

src/server/db_slice.cc

-  events_.evicted_keys += evicted_items;
-  DVLOG(2) << "Eviction time (us): " << (time_finish - time_start) / 1000;
-  return pair<uint64_t, size_t>{evicted_items, evicted_bytes};
+  return finalize();


no need to call finalize if expired_keys_events_recording_ is false and journal is null

actually it's better to check in finalize to avoid iterating over keys_to_journal

bool record_keys = owner_->journal() != nullptr || expired_keys_events_recording_;

and then:

1388 if (record_keys) 1389 keys_to_journal.emplace_back(key);

So keys_to_journal will be empty if either we have key space notifications or if we need to write to the journal.

So no action really needed there because keys_to_journal will be empty.

What we can save is calling SendQueuedInvalidationMessagesAsync if pending_send_map.empty()

romange · 2025-04-28T14:28:59Z

src/server/db_slice.cc

+    // send the deletion to the replicas.
+    for (string_view key : keys_to_journal) {
+      if (auto journal = owner_->journal(); journal)
+        RecordExpiryBlocking(db_ind, key);


please add a comment saying that even though you call RecordExpiryBlocking it does not block due to JournalFlushGuard above

I find funny that Adi asked me why I used

// Disable flush journal changes to prevent preemtion journal::JournalFlushGuard journal_flush_guard(shard_owner()->journal());

when the caller of this function already does the same. Then you are asking me to add a comment here.

yet the only reason I added the line above on the first line of this function is precisely to show intent (that we disable the flushing in journal explicitly).

I will add a comment 😄

adiholden · 2025-04-28T19:21:21Z

src/server/db_slice.cc

+                                                              size_t starting_segment_id,
+                                                              size_t increase_goal_bytes) {
+  // Disable flush journal changes to prevent preemtion
+  journal::JournalFlushGuard journal_flush_guard(shard_owner()->journal());


The caller to this function also uses journal::JournalFlushGuard
In this class distructor we do journal_->SetFlushMode(true);
This means that the logic now is broken if there are other calls to journal not inside FreeMemWithEvictionStepAtomic

I would add a dcheck in JournalFlushGuard to see we do not have nested calls to it.

why did you add it here if its also in the caller?

kostasrim · 2025-04-29T09:11:57Z

src/server/db_slice.cc

+           ++num_seg_visited, segment_id = GetNextSegmentForEviction(segment_id, db_ind)) {
+        const auto& bucket = db_table->prime.GetSegment(segment_id)->GetBucket(bucket_id);
+        if (bucket.IsEmpty() || !bucket.IsBusy(slot_id))
+          continue;


Only difference is I combined those two statements

kostasrim · 2025-04-29T09:12:15Z

src/server/db_slice.cc

-          if ((evicted_items == max_eviction_per_hb) || (evicted_bytes >= increase_goal_bytes))
-            goto finish;
-        }
+  for (int32_t slot_id = num_slots - 1; slot_id >= 0; --slot_id) {


No functional changes in this block of code. I moved the FiberAtomicGuard above

@adiholden I can also move back the Guard locally so the diff is cleaner

kostasrim self-assigned this Apr 3, 2025

kostasrim commented Apr 3, 2025

View reviewed changes

kostasrim changed the title ~~[wip] fix: make FreeMemWithEvictionStep atomic~~ fix: make FreeMemWithEvictionStep atomic Apr 11, 2025

fix: make FreeMemWithEvictionStep atomic

26764f7

Signed-off-by: kostas <[email protected]>

kostasrim force-pushed the kpr4 branch from ae988c5 to 26764f7 Compare April 11, 2025 11:06

kostasrim requested review from adiholden and romange April 11, 2025 11:07

Merge branch 'main' into kpr4

41ca91f

BorysTheDev reviewed Apr 28, 2025

View reviewed changes

romange reviewed Apr 28, 2025

View reviewed changes

adiholden reviewed Apr 28, 2025

View reviewed changes

make great goto great again

68c83ae

kostasrim commented Apr 29, 2025

View reviewed changes

kostasrim requested review from adiholden, romange and BorysTheDev April 29, 2025 09:14

adiholden approved these changes Apr 29, 2025

View reviewed changes

kostasrim merged commit 291b262 into main May 2, 2025
10 checks passed

kostasrim deleted the kpr4 branch May 2, 2025 07:31

fix: make FreeMemWithEvictionStep atomic #4885

fix: make FreeMemWithEvictionStep atomic #4885

Uh oh!

Conversation

kostasrim commented Apr 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kostasrim Apr 11, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

kostasrim Apr 28, 2025 • 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!

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!

kostasrim Apr 29, 2025 • 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!

Uh oh!

Uh oh!

kostasrim Apr 11, 2025 •

edited

Loading

kostasrim Apr 28, 2025 •

edited

Loading

kostasrim Apr 29, 2025 •

edited

Loading