mlcommons / mlcommons/storage

KVCache: eviction-driven tier I/O is excluded from latency P95 SLA checks (Storage/CPU write & read latencies never recorded in _demote_entry)

Open
#421 2 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Future KVCache TF
Dominant language
Python
Stars
203
Forks
67
Avg merge
20m
Merged PRs (30d)
8

Description

Summary

This is a sibling of #408 (fixed by #420) on the latency side. When the KV cache evicts data from a higher tier into a lower one, kv_cache/cache.py::_demote_entry() performs real backend reads and writes but never appends the resulting timings to the per-tier latency lists (storage_write_latencies, storage_read_latencies, cpu_write_latencies, cpu_read_latencies, and the storage_*_device/host_latencies variants).

As a result, on the typical multi-tier setup where a tier is populated via eviction rather than direct allocation, _evaluate_storage_performance() (default performance_profile='latency') finds an empty storage_write_basis and silently skips the "Storage Tier Write P95 < 500ms" SLA criterion (and similarly the read P95 / CPU RAM P95 criteria). The check doesn't fail — it simply isn't evaluated, so a submission can omit the most important storage-latency gate without any signal.

#408 / #420 fixed the byte counters (tier_*_kv_bytes_written/read, and the throughput-profile bandwidth derived from them). This issue tracks the analogous latency gap, which #420 was intentionally kept minimal to exclude.

Root cause

_demote_entry() computes read_timing and write_timing from the backend calls and returns their sum, but the caller discards it (success, _ = self._demote_entry(...)), and the method never appends to the latency lists that _evaluate_storage_performance() consumes:

data, read_timing  = self.backends[from_tier].read(key)
write_timing       = self.backends[to_tier].write(key, data)
self.backends[from_tier].delete(key)
...
with self.stats_lock:
    self.stats['evictions'] += 1
    # bytes are now tracked (PR #420), but NOT latencies:
    # MISSING: append read_timing  to tier_{from}_*_latencies
    # MISSING: append write_timing to tier_{to}_*_latencies

By contrast, the direct-allocation path (_allocate_cache_inner) and the read path (access_cache) both record latencies, e.g.:

# _allocate_cache_inner (nvme branch)
self.stats['storage_write_latencies'].append(timing.total)
self.stats['storage_write_device_latencies'].append(timing.device)
self.stats['storage_write_host_latencies'].append(timing.host)

Impact

  • Default (latency) profile: the Storage Tier Write/Read P95 SLA criteria are skipped entirely on eviction-driven multi-tier runs — the same configurations that motivated #408.
  • The discrepancy is silent: storage_health reports passed_count/total_count over only the criteria that happened to have data.

Suggested fix

Inside the existing with self.stats_lock: block in _demote_entry(), append the timings symmetric to the byte counters added in #420 (mirroring _allocate_cache_inner / access_cache):

# read out of from_tier
if from_tier == 'gpu':
    self.stats['gpu_read_latencies'].append(read_timing.total)
elif from_tier == 'cpu':
    self.stats['cpu_read_latencies'].append(read_timing.total)
elif from_tier == 'nvme':
    self.stats['storage_read_latencies'].append(read_timing.total)
    self.stats['storage_read_device_latencies'].append(read_timing.device)
    self.stats['storage_read_host_latencies'].append(read_timing.host)

# write into to_tier
if to_tier == 'cpu':
    self.stats['cpu_write_latencies'].append(write_timing.total)
elif to_tier == 'nvme':
    self.stats['storage_write_latencies'].append(write_timing.total)
    self.stats['storage_write_device_latencies'].append(write_timing.device)
    self.stats['storage_write_host_latencies'].append(write_timing.host)

A regression test analogous to the one in #420 (in TestThreeTierEvictionCascade) should assert that, after an eviction-driven cascade, storage_write_latencies / cpu_*_latencies are non-empty and that _evaluate_storage_performance() actually evaluates the Storage Write/Read P95 criteria.

Notes

Found while reviewing the #408 fix (#420) with a multi-model code-review panel (GPT-5.5, Claude Opus 4.8, Gemini 3.1 Pro). Filed separately to keep #420 scoped to the byte-counter fix the WG already approved.

Environment

  • Bench: kv_cache_benchmark pyproject.toml version 3.0.0 (current main)

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in kv_cache/cache.py::_demote_entry() and compare its timing handling with _allocate_cache_inner and access_cache. Run the regression coverage in TestThreeTierEvictionCascade, then verify eviction-driven cascades populate the per-tier latency lists and that _evaluate_storage_performance() evaluates the Storage Write/Read P95 criteria.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend, performance
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.