kvcache-ai/Mooncake

Fork 0

mirror of https://github.com/kvcache-ai/Mooncake.git synced 2026-11-05 01:17:55 +00:00

[GH-ISSUE #2528] [TENT] Priority promotion decides on queue head but promotes the whole tier #672

New issue

Closed

opened 2026-10-29 21:17:55 +00:00 by bsakaguchi · 4 comments

bsakaguchi commented

2026-10-29 21:17:55 +00:00

Owner

Originally created by @catyans on GitHub (Jun 18, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/2528

Originally assigned to: @catyans on GitHub.

Describe the issue

While reading the TENT worker priority-promotion logic
(mooncake-transfer-engine/tent/src/transport/rdma/workers.cpp,
Workers::promoteTimedOutRequests, around L393-434), I noticed a few
things in the starvation-prevention path that look unintended. Filing to
confirm whether these are by design before proposing a fix.

The relevant block (MEDIUM→HIGH; LOW→MEDIUM is symmetric):

std::vector<RdmaSliceList> promoted;
worker.queues[PRIO_MEDIUM].pop(promoted);          // drains the WHOLE queue
if (!promoted.empty()) {
    auto* slice = promoted.front().first;          // only inspects the HEAD
    if (slice && slice->enqueue_ts > 0 &&
        (current_ts - slice->enqueue_ts) >= priority_promotion_timeout_ns_) {
        for (auto& slice_list : promoted)
            worker.queues[PRIO_HIGH].push(slice_list);   // promote ALL
        return;
    }
    for (auto& slice_list : promoted)
        worker.queues[PRIO_MEDIUM].push(slice_list);     // push ALL back
}

1. Promotion decision is made from the head element only, but applied to the whole queue.
pop(promoted) drains the entire MEDIUM queue, then only
promoted.front()'s enqueue_ts is checked. If the head is timed out,
every MEDIUM entry is promoted to HIGH — including ones that were just
enqueued and aren't starving. Conversely, if the head is not timed out
but later entries are, none are promoted this round.

2. Only one level is promoted per call.
When MEDIUM→HIGH fires, the function returns, so the LOW→MEDIUM check is
skipped for that 1ms tick. A LOW entry needs multiple promotion cycles to
climb to HIGH. (Minor, but worth noting alongside #1.)

3. Drain-then-refill on the non-timeout path.
When the head is not timed out, the code still pops the entire queue and
pushes every element back every ~1ms. pop/push preserve FIFO order
in isolation, so ordering across a single quiet tick is fine — but since
the queue is MPSC (multiple producers can push concurrently during the
drain/refill window), interleaving the drained refill with concurrent
producer pushes can change the relative order, and the full drain+refill
is pure overhead on every idle tick.

Question

Is the whole-queue promotion driven by the head element intentional
(e.g. a deliberate coarse-grained "if the oldest is starving, flush the
tier" policy), or is the intended behavior to promote only the entries
that have actually exceeded priority_promotion_timeout_ns_?

If per-entry (or at least non-draining) promotion is preferred, I'm happy
to put up a small PR — wanted to confirm the design intent first since
this is starvation-prevention behavior.

(To be clear: I checked BoundedMPSCQueue::pop(vector)/push and they do
not reverse order on their own, so this is not a FIFO-reversal bug —
the concern is the head-only decision + whole-queue effect + the
per-tick drain/refill, not order inversion.)

Environment

Reading main source (TENT transport).
N/A — code-review observation, not a runtime failure.

Originally created by @catyans on GitHub (Jun 18, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/2528 Originally assigned to: @catyans on GitHub. ### Describe the issue While reading the TENT worker priority-promotion logic (`mooncake-transfer-engine/tent/src/transport/rdma/workers.cpp`, `Workers::promoteTimedOutRequests`, around L393-434), I noticed a few things in the starvation-prevention path that look unintended. Filing to confirm whether these are by design before proposing a fix. The relevant block (MEDIUM→HIGH; LOW→MEDIUM is symmetric): ```cpp std::vector<RdmaSliceList> promoted; worker.queues[PRIO_MEDIUM].pop(promoted); // drains the WHOLE queue if (!promoted.empty()) { auto* slice = promoted.front().first; // only inspects the HEAD if (slice && slice->enqueue_ts > 0 && (current_ts - slice->enqueue_ts) >= priority_promotion_timeout_ns_) { for (auto& slice_list : promoted) worker.queues[PRIO_HIGH].push(slice_list); // promote ALL return; } for (auto& slice_list : promoted) worker.queues[PRIO_MEDIUM].push(slice_list); // push ALL back } ``` **1. Promotion decision is made from the head element only, but applied to the whole queue.** `pop(promoted)` drains the entire MEDIUM queue, then only `promoted.front()`'s `enqueue_ts` is checked. If the head is timed out, *every* MEDIUM entry is promoted to HIGH — including ones that were just enqueued and aren't starving. Conversely, if the head is *not* timed out but later entries are, none are promoted this round. **2. Only one level is promoted per call.** When MEDIUM→HIGH fires, the function `return`s, so the LOW→MEDIUM check is skipped for that 1ms tick. A LOW entry needs multiple promotion cycles to climb to HIGH. (Minor, but worth noting alongside #1.) **3. Drain-then-refill on the non-timeout path.** When the head is not timed out, the code still `pop`s the entire queue and `push`es every element back every ~1ms. `pop`/`push` preserve FIFO order in isolation, so ordering across a single quiet tick is fine — but since the queue is MPSC (multiple producers can `push` concurrently during the drain/refill window), interleaving the drained refill with concurrent producer pushes can change the relative order, and the full drain+refill is pure overhead on every idle tick. ### Question Is the **whole-queue promotion driven by the head element** intentional (e.g. a deliberate coarse-grained "if the oldest is starving, flush the tier" policy), or is the intended behavior to promote only the entries that have actually exceeded `priority_promotion_timeout_ns_`? If per-entry (or at least non-draining) promotion is preferred, I'm happy to put up a small PR — wanted to confirm the design intent first since this is starvation-prevention behavior. (To be clear: I checked `BoundedMPSCQueue::pop(vector)`/`push` and they do **not** reverse order on their own, so this is *not* a FIFO-reversal bug — the concern is the head-only decision + whole-queue effect + the per-tick drain/refill, not order inversion.) ### Environment - Reading `main` source (TENT transport). - N/A — code-review observation, not a runtime failure.

bsakaguchi closed this issue

2026-10-29 21:17:55 +00:00

bsakaguchi commented

2026-10-29 21:17:56 +00:00

Author

Owner

@github-actions[bot] commented on GitHub (Jun 18, 2026):

Thanks for opening this issue, @catyans!

Field	Value
Issue	#2528
GitHub user ID	`18214026`
Reporter	@catyans

A maintainer will triage this when possible. To help us respond faster, please include:

Mooncake version or commit SHA
Environment (OS, CUDA/driver, RDMA stack if relevant)
Steps to reproduce and expected vs. actual behavior

Useful links: Documentation · Contributing guide

This message was posted automatically by the issue bot.

@github-actions[bot] commented on GitHub (Jun 18, 2026): Thanks for opening this issue, @catyans! | Field | Value | |-------|-------| | **Issue** | #2528 | | **GitHub user ID** | `18214026` | | **Reporter** | @catyans | A maintainer will triage this when possible. To help us respond faster, please include: - Mooncake version or commit SHA - Environment (OS, CUDA/driver, RDMA stack if relevant) - Steps to reproduce and expected vs. actual behavior Useful links: [Documentation](https://kvcache-ai.github.io/Mooncake/) · [Contributing guide](https://github.com/kvcache-ai/Mooncake/blob/main/CONTRIBUTING.md) > This message was posted automatically by the issue bot.

bsakaguchi commented

2026-10-29 21:17:56 +00:00

Author

Owner

@alogfans commented on GitHub (Jun 24, 2026):

Thanks for the detailed analysis — this is a thoughtful and well-articulated review.

To answer your question: the current behavior is intentional, as this was a simple starvation-prevention strategy we adopted in the initial implementation. The rationale is:

Coarse-grained "flush the tier" policy: If the oldest entry in a queue has been waiting long enough to time out, it suggests the system is under enough pressure that promoting the entire batch is a reasonable safeguard. This prioritizes worst-case latency over per-fairness granularity.
Simple is fast: Checking only the head element avoids iterating through the entire queue every 1ms, which would add overhead. The trade-off is acceptable for our current workloads.
Drain-refill on timeout path: The MPSC queue order concern is valid, but in practice the promotion window is short enough that concurrent producer interleaving hasn't been a problem in our testing.

That said, you're right that this could be improved — particularly the head-only decision logic and the per-tick drain-refill overhead. For now, this simple strategy meets our needs, but we're open to refining it if real-world scenarios demonstrate issues.

@alogfans commented on GitHub (Jun 24, 2026): Thanks for the detailed analysis — this is a thoughtful and well-articulated review. To answer your question: the current behavior is **intentional**, as this was a simple starvation-prevention strategy we adopted in the initial implementation. The rationale is: 1. **Coarse-grained "flush the tier" policy**: If the oldest entry in a queue has been waiting long enough to time out, it suggests the system is under enough pressure that promoting the entire batch is a reasonable safeguard. This prioritizes worst-case latency over per-fairness granularity. 2. **Simple is fast**: Checking only the head element avoids iterating through the entire queue every 1ms, which would add overhead. The trade-off is acceptable for our current workloads. 3. **Drain-refill on timeout path**: The MPSC queue order concern is valid, but in practice the promotion window is short enough that concurrent producer interleaving hasn't been a problem in our testing. That said, you're right that this could be improved — particularly the head-only decision logic and the per-tick drain-refill overhead. For now, this simple strategy meets our needs, but we're open to refining it if real-world scenarios demonstrate issues.

bsakaguchi commented

2026-10-29 21:17:56 +00:00

Author

Owner

@catyans commented on GitHub (Jul 7, 2026):

Following up with a deterministic, self-contained reproduction of the two behaviors from the original report, rather than noisy end-to-end latency numbers. I factored the promotion decision out of promoteTimedOutRequests into a small pure helper (promotion_policy.h) so it can be exercised without the RDMA stack or timing jitter — the decision only depends on each entry's enqueue_ts, so this is faithful to the real logic.

Two failing scenarios (both compiled + run, gtest):

1. Head-only decision, whole-queue promotion. Queue = [head=timed-out, fresh, fresh] (the two behind the head were just enqueued):

historical head-only policy promotes indices {0, 1, 2} — the two fresh, non-starving entries get promoted to HIGH alongside the starving head;
a per-entry policy promotes only {0} (the genuinely starving one).

2. Timed-out tail missed when head is fresh. Queue = [head=fresh, timed-out, timed-out]:

historical head-only policy promotes {} — nothing, even though indices 1 and 2 have been starving;
per-entry policy promotes {1, 2}.

Both policies agree on the two cases your design targets (all-timed-out → promote all; empty / zero-timestamp → promote none), so a per-entry policy is a strict refinement of the "flush the tier" intent, not a behavior change for the under-pressure case you optimized for.

I understand the head-only check was a deliberate cost trade-off (avoid scanning the queue every 1ms). The scan is bounded by queue depth and runs once per ~1ms tick, so per-entry selection is cheap; if you'd rather keep it opt-in I can gate it behind a config flag (default = today's behavior). Would you accept a PR that (a) keeps the current policy as default and (b) adds an opt-in per-entry policy + this test? Happy to also fix the "only one level promoted per tick" return in the same change if wanted.

Repro branch: exp/promotion-2528 on my fork (promotion_policy.h + promotion_policy_test.cpp).

@catyans commented on GitHub (Jul 7, 2026): Following up with a **deterministic, self-contained reproduction** of the two behaviors from the original report, rather than noisy end-to-end latency numbers. I factored the promotion *decision* out of `promoteTimedOutRequests` into a small pure helper (`promotion_policy.h`) so it can be exercised without the RDMA stack or timing jitter — the decision only depends on each entry's `enqueue_ts`, so this is faithful to the real logic. Two failing scenarios (both compiled + run, gtest): **1. Head-only decision, whole-queue promotion.** Queue = `[head=timed-out, fresh, fresh]` (the two behind the head were just enqueued): - historical head-only policy promotes indices `{0, 1, 2}` — the two **fresh, non-starving** entries get promoted to HIGH alongside the starving head; - a per-entry policy promotes only `{0}` (the genuinely starving one). **2. Timed-out tail missed when head is fresh.** Queue = `[head=fresh, timed-out, timed-out]`: - historical head-only policy promotes `{}` — nothing, even though indices 1 and 2 have been starving; - per-entry policy promotes `{1, 2}`. Both policies agree on the two cases your design targets (all-timed-out → promote all; empty / zero-timestamp → promote none), so a per-entry policy is a strict refinement of the "flush the tier" intent, not a behavior change for the under-pressure case you optimized for. I understand the head-only check was a deliberate cost trade-off (avoid scanning the queue every 1ms). The scan is bounded by queue depth and runs once per ~1ms tick, so per-entry selection is cheap; if you'd rather keep it opt-in I can gate it behind a config flag (default = today's behavior). Would you accept a PR that (a) keeps the current policy as default and (b) adds an opt-in per-entry policy + this test? Happy to also fix the "only one level promoted per tick" `return` in the same change if wanted. Repro branch: `exp/promotion-2528` on my fork (`promotion_policy.h` + `promotion_policy_test.cpp`).

bsakaguchi commented

2026-10-29 21:17:56 +00:00

Author

Owner

@catyans commented on GitHub (Jul 8, 2026):

Opened #2788 with an opt-in fix, @alogfans. It keeps your head-only 'flush the tier' policy as the default (byte-for-byte, one level per tick) and adds transports/rdma/priority_promotion_per_entry (default false) to switch to per-entry promotion. The decision logic is factored into promotion_policy.h with the deterministic repro from the comment above turned into a unit test. Nothing changes unless the flag is set — happy to adjust the seam or drop the LOW→MEDIUM same-tick change if you'd prefer it stay strictly one-level-per-tick even when opted in.

@catyans commented on GitHub (Jul 8, 2026): Opened #2788 with an **opt-in** fix, @alogfans. It keeps your head-only 'flush the tier' policy as the default (byte-for-byte, one level per tick) and adds `transports/rdma/priority_promotion_per_entry` (default false) to switch to per-entry promotion. The decision logic is factored into `promotion_policy.h` with the deterministic repro from the comment above turned into a unit test. Nothing changes unless the flag is set — happy to adjust the seam or drop the LOW→MEDIUM same-tick change if you'd prefer it stay strictly one-level-per-tick even when opted in.