Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions crates/grpc_client/proto/vllm_engine.proto
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,22 @@ message TokenizedInput {
repeated uint32 input_ids = 2; // Actual token IDs to process
}

// A typed tensor: raw little-endian bytes + shape + dtype.
// A typed tensor descriptor. The raw bytes live in exactly one payload
// transport; shape and dtype describe how the receiver interprets them.
message TensorData {
bytes data = 1; // Raw little-endian bytes (f32/i64/u32)
repeated uint32 shape = 2; // Dimension sizes
string dtype = 3; // "float32", "int64", "uint32"

oneof payload {
// Current path: raw little-endian bytes carried in the gRPC message
// (field 1, formerly `bytes data`, kept for wire compatibility).
bytes inline = 1;
// Same-host CPU shared memory path; preferred for large payloads when
// SMG and the vLLM worker share /dev/shm.
smg.grpc.common.ShmHandle shm = 4;
// Cross-node or non-shared-memory transport (e.g. NIXL). Not implemented.
smg.grpc.common.RemoteTensorHandle remote = 5;
}
}

message PlaceholderRange {
Expand Down Expand Up @@ -339,6 +350,11 @@ message GetServerInfoResponse {
string kv_engine_id = 8; // kv_transfer_config.engine_id, "" if not configured

int32 data_parallel_size = 9; // parallel_config.data_parallel_size (1 when DP is off)

// This worker's /dev/shm tmpfs identity (<boot_id>:<st_dev>), advertised so
// the router can verify a shared /dev/shm before using the SHM tensor
// transport under `auto`. Empty when it can't be determined.
string shm_namespace_id = 10;
}

// =====================
Expand Down
100 changes: 100 additions & 0 deletions grpc_servicer/smg_grpc_servicer/mm_shm.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
"""Shared multimodal SHM tensor-transport helpers (engine-neutral).

Reads a ``TensorData`` payload that carries its raw little-endian bytes either
inline in the gRPC message or via a same-host ``/dev/shm`` handle, and reports
this process's ``/dev/shm`` namespace identity. Used by the vLLM and TokenSpeed
gRPC servicers; their ``TensorData``/``ShmHandle`` messages share the ``payload``
oneof shape (``inline`` | ``shm`` | ``remote``) via ``common.proto``.
"""

import os
Comment thread
coderabbitai[bot] marked this conversation as resolved.
import stat

# Unlink each /dev/shm segment right after the worker reads it (default on) so
# same-host SHM tensors don't accumulate. Disable with
# TOKENSPEED_UNLINK_MM_SHM_AFTER_READ=0 (e.g. for debugging).
UNLINK_MM_SHM_AFTER_READ = os.getenv("TOKENSPEED_UNLINK_MM_SHM_AFTER_READ", "1").lower() not in (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: The env var is TOKENSPEED_UNLINK_MM_SHM_AFTER_READ but this module is engine-neutral (used by both vLLM and TokenSpeed servicers). A SMG_UNLINK_MM_SHM_AFTER_READ name would be consistent with the module's scope. Not urgent — fine to defer if you plan to rename during the TokenSpeed servicer migration (mentioned in follow-ups).

"0",
"false",
"no",
)


def tensor_payload_bytes(tensor_data) -> bytes:
"""Raw bytes of a ``TensorData``, from whichever payload transport it carries."""
payload = tensor_data.WhichOneof("payload")
if payload == "inline":
return bytes(tensor_data.inline)
if payload == "shm":
return tensor_payload_bytes_from_shm(tensor_data.shm)
if payload == "remote":
raise ValueError("TensorData.remote payload is not implemented yet")
raise ValueError("TensorData payload is required")


def tensor_payload_bytes_from_shm(shm_handle) -> bytes:
"""Read ``nbytes`` from ``/dev/shm/<name>`` at ``offset`` (unlinking after read
when enabled)."""
name = validated_shm_name(shm_handle.name)

path = os.path.join("/dev/shm", name)
fd = None
try:
# O_NOFOLLOW: never follow a symlink at the final path component, so a
# crafted name that resolves to a pre-existing symlink in /dev/shm can't
# redirect the read. Then require a regular file (fail closed otherwise).
fd = os.open(path, os.O_RDONLY | os.O_NOFOLLOW)
if not stat.S_ISREG(os.fstat(fd).st_mode):
raise ValueError(f"TensorData.shm is not a regular file: {shm_handle.name!r}")
Comment on lines +46 to +48

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

sed -n '1,220p' grpc_servicer/smg_grpc_servicer/mm_shm.py

Repository: lightseekorg/smg

Length of output: 1955


🏁 Script executed:

python3 - <<'PY'
import os, tempfile, multiprocessing, time, errno, stat, sys

td = tempfile.mkdtemp(prefix="fifo-probe-")
fifo = os.path.join(td, "x")
reg = os.path.join(td, "r")
os.mkfifo(fifo, 0o600)
with open(reg, "wb") as f:
    f.write(b"hi")

def try_open(path, flags, q):
    try:
        fd = os.open(path, flags)
        try:
            mode = stat.S_IFMT(os.fstat(fd).st_mode)
            q.put(("ok", mode))
        finally:
            os.close(fd)
    except Exception as e:
        q.put(("err", type(e).__name__, getattr(e, "errno", None), str(e)))

def run(path, flags, timeout=1.0):
    q = multiprocessing.Queue()
    p = multiprocessing.Process(target=try_open, args=(path, flags, q))
    p.start()
    p.join(timeout)
    if p.is_alive():
        p.terminate()
        p.join()
        return ("timeout",)
    return q.get() if not q.empty() else ("no-result",)

cases = [
    ("fifo_rd", fifo, os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0)),
    ("fifo_rd_nonblock", fifo, os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) | os.O_NONBLOCK),
    ("reg_rd", reg, os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0)),
    ("reg_rd_nonblock", reg, os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) | os.O_NONBLOCK),
]
for name, path, flags in cases:
    print(name, run(path, flags))
PY

Repository: lightseekorg/smg

Length of output: 1955


Avoid blocking FIFO opens

os.open(path, os.O_RDONLY | os.O_NOFOLLOW) can still block if /dev/shm contains a FIFO at path. Add O_NONBLOCK so the regular-file check can reject non-regular targets without hanging.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@grpc_servicer/smg_grpc_servicer/mm_shm.py` around lines 46 - 48, Avoid
blocking when opening shared-memory paths that may point to FIFOs. In mm_shm.py,
update the os.open call in the TensorData.shm path handling to include
O_NONBLOCK alongside O_RDONLY and O_NOFOLLOW, so the subsequent stat.S_ISREG
check can safely reject non-regular files without hanging. Keep the fix
localized to the same open-and-validate logic that uses shm_handle.name and
os.fstat(fd).

raw = os.pread(fd, int(shm_handle.nbytes), int(shm_handle.offset))
finally:
if fd is not None:
os.close(fd)
if fd is not None and UNLINK_MM_SHM_AFTER_READ:
try:
os.unlink(path)
except FileNotFoundError:
pass

if len(raw) != int(shm_handle.nbytes):
raise ValueError(
f"TensorData.shm byte length mismatch for name={shm_handle.name!r}: "
f"expected {int(shm_handle.nbytes)}, got {len(raw)}"
)
return raw


def validated_shm_name(name: str) -> str:
"""Reject path-traversal / absolute / empty SHM names before opening."""
name = name.lstrip("/")
if not name or "/" in name or name in (".", "..") or "\x00" in name:
raise ValueError(f"Invalid TensorData.shm name: {name!r}")
return name


_shm_namespace_id_cache: str | None = None


def shm_namespace_id() -> str:
"""Identity of this process's ``/dev/shm`` tmpfs: ``<boot_id>:<st_dev>``.

``boot_id`` (``/proc/sys/kernel/random/boot_id``) is not namespaced, so it
pins the host; ``st_dev`` is the tmpfs superblock device backing
``/dev/shm``. Two processes share ``/dev/shm`` iff both match -- including
separate containers sharing it via ``--ipc``/bind-mount, where mount
namespaces differ but the underlying superblock (``st_dev``) is the same. The
router compares this token to its own to decide the SHM tensor transport.
Empty string if it can't be determined. Cached: both components are static
for the process lifetime, and this is read on every GetServerInfo.
"""
global _shm_namespace_id_cache
if _shm_namespace_id_cache is not None:
return _shm_namespace_id_cache
try:
with open("/proc/sys/kernel/random/boot_id", encoding="ascii") as f:
boot_id = f.read().strip()
shm_dev = os.stat("/dev/shm").st_dev
_shm_namespace_id_cache = f"{boot_id}:{shm_dev}"
except OSError:
_shm_namespace_id_cache = ""
return _shm_namespace_id_cache
5 changes: 4 additions & 1 deletion grpc_servicer/smg_grpc_servicer/vllm/servicer.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
from vllm.outputs import CompletionOutput, RequestOutput
from vllm.sampling_params import RequestOutputKind, StructuredOutputsParams

from smg_grpc_servicer import mm_shm
from smg_grpc_servicer.tokenizer_bundle import CHUNK_SIZE, build_tokenizer_zip
from smg_grpc_servicer.vllm.kv_events import (
endpoint_for_rank,
Expand Down Expand Up @@ -78,7 +79,8 @@ def _tensor_from_proto(td: vllm_engine_pb2.TensorData) -> torch.Tensor:
torch_dtype = _PROTO_DTYPE_MAP.get(td.dtype)
if torch_dtype is None:
raise ValueError(f"Unsupported proto tensor dtype: {td.dtype!r}")
return torch.frombuffer(bytearray(td.data), dtype=torch_dtype).reshape(*td.shape)
payload = mm_shm.tensor_payload_bytes(td)
return torch.frombuffer(bytearray(payload), dtype=torch_dtype).reshape(*td.shape)
Comment on lines +82 to +83

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using bytearray(payload) creates a mutable copy of the entire tensor data in CPU memory, which introduces an unnecessary copy and defeats the zero-copy benefit of the shared memory (SHM) transport. Since torch.frombuffer natively supports read-only buffers (like bytes) and returns a read-only tensor, you can pass payload directly to torch.frombuffer to achieve true zero-copy deserialization.

Suggested change
payload = mm_shm.tensor_payload_bytes(td)
return torch.frombuffer(bytearray(payload), dtype=torch_dtype).reshape(*td.shape)
payload = mm_shm.tensor_payload_bytes(td)
return torch.frombuffer(payload, dtype=torch_dtype).reshape(*td.shape)



try:
Expand Down Expand Up @@ -487,6 +489,7 @@ async def GetServerInfo(
kv_role=kv_role,
kv_engine_id=kv_engine_id,
data_parallel_size=parallel.data_parallel_size,
shm_namespace_id=mm_shm.shm_namespace_id(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Require a proto release that contains the SHM fields

This servicer now constructs GetServerInfoResponse with shm_namespace_id, and _tensor_from_proto also calls WhichOneof("payload"), but grpc_servicer/pyproject.toml still allows smg-grpc-proto>=0.4.11 while the proto package version in this repo was not bumped past the pre-change 0.4.12. A normal package install can therefore pair the new servicer with generated stubs that do not define these fields, causing worker registration to raise TypeError: Protocol message GetServerInfoResponse has no "shm_namespace_id" field before SHM is even negotiated. Please bump/pin smg-grpc-proto to a new release before using the new field.

Useful? React with 👍 / 👎.

)

async def GetLoads(
Expand Down
56 changes: 32 additions & 24 deletions model_gateway/src/routers/grpc/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ use smg_grpc_client::{

use crate::routers::grpc::{
proto_wrapper::{
cleanup_tokenspeed_shm_handles, collect_tokenspeed_generate_request_shm_handles,
finish_tokenspeed_request, ProtoEmbedComplete, ProtoEmbedRequest, ProtoGenerateRequest,
ProtoStream,
cleanup_mm_shm_handles, collect_tokenspeed_generate_request_shm_handles,
collect_vllm_generate_request_shm_handles, finish_tokenspeed_request, finish_vllm_request,
ProtoEmbedComplete, ProtoEmbedRequest, ProtoGenerateRequest, ProtoStream,
},
MultimodalData,
};
Expand Down Expand Up @@ -397,8 +397,14 @@ impl GrpcClient {
Ok(ProtoStream::Sglang(stream))
}
(Self::Vllm(client), ProtoGenerateRequest::Vllm(boxed_req)) => {
let stream = client.generate(*boxed_req).await?;
Ok(ProtoStream::Vllm(stream))
let shm_handles = collect_vllm_generate_request_shm_handles(&boxed_req);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Important: SHM cleanup covers the send-failure path here, but the build-failure path is unguarded. When into_proto() writes SHM segments during build_generate_request (e.g. at client.rs:495), then build_grpc_sampling_params_from_chat fails via ?, the SHM files leak — the segments are dropped without cleanup and generate() never runs.

TokenSpeed handles this with finish_tokenspeed_request (proto_wrapper.rs:675), which collects SHM handles before the build and cleans up on Err. The vLLM path needs an equivalent guard — either a finish_vllm_request wrapper or an explicit collect-and-cleanup around the into_proto() + build sequence in each build_generate_request arm.

match client.generate(*boxed_req).await {
Ok(stream) => Ok(ProtoStream::Vllm(stream)),
Err(error) => {
cleanup_mm_shm_handles(&shm_handles);
Err(error)
}
}
}
(Self::Trtllm(client), ProtoGenerateRequest::Trtllm(boxed_req)) => {
let stream = client.generate(*boxed_req).await?;
Expand All @@ -413,7 +419,7 @@ impl GrpcClient {
match client.generate(*boxed_req).await {
Ok(stream) => Ok(ProtoStream::TokenSpeed(stream)),
Err(error) => {
cleanup_tokenspeed_shm_handles(&shm_handles);
cleanup_mm_shm_handles(&shm_handles);
Err(error)
}
}
Expand Down Expand Up @@ -489,15 +495,16 @@ impl GrpcClient {
MultimodalData::Vllm(data) => data.into_proto(),
_ => unreachable!("caller guarantees matching variant"),
});
let req = client.build_generate_request_from_chat(
request_id,
body,
processed_text,
token_ids,
vllm_mm,
options.tool_constraints,
)?;
Ok(ProtoGenerateRequest::Vllm(Box::new(req)))
finish_vllm_request(vllm_mm, |mm| {
client.build_generate_request_from_chat(
request_id,
body,
processed_text,
token_ids,
mm,
options.tool_constraints,
)
})
}
Self::Trtllm(client) => {
let trtllm_mm = options.multimodal_inputs.map(|mm| match mm {
Expand Down Expand Up @@ -580,15 +587,16 @@ impl GrpcClient {
MultimodalData::Vllm(data) => data.into_proto(),
_ => unreachable!("caller guarantees matching variant"),
});
let req = client.build_generate_request_from_messages(
request_id,
body,
processed_text,
token_ids,
vllm_mm,
options.tool_constraints,
)?;
Ok(ProtoGenerateRequest::Vllm(Box::new(req)))
finish_vllm_request(vllm_mm, |mm| {
client.build_generate_request_from_messages(
request_id,
body,
processed_text,
token_ids,
mm,
options.tool_constraints,
)
})
}
Self::Trtllm(client) => {
let trtllm_mm = options.multimodal_inputs.map(|mm| match mm {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,11 @@ impl RequestExecutionStage {
// Decode reuses proto_request as-is; same request_id as the prefill leg is
// load-bearing for NIXL P/D correlation on vLLM < 0.13
let mut decode_request = proto_request;
// Decode doesn't run the vision encoder (it receives KV via the P/D
// transfer), so drop the multimodal inputs — mirrors the parallel PD
// path. Load-bearing for SHM: prefill already read and unlinked the
// /dev/shm segments, so a reused ShmHandle here would be unreadable.
decode_request.clear_mm_pixel_values();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep multimodal inputs when decode must recompute

For vLLM PD requests with sampling n > 1, relay_kv_params is false, so the decode leg is intentionally not handed prefill KV and the NIXL path logs that decode will recompute the prompt locally. Clearing mm_inputs here means a multimodal decode request reaches vLLM with the expanded image tokens but without the image tensors/model-specific tensors, so NIXL n > 1 image requests will fail or generate from an incomplete prompt. Only drop these tensors on paths where decode actually consumes transferred KV, or give the recompute path its own payloads.

Useful? React with 👍 / 👎.

if let Some(rank) = workers.decode_worker().and_then(|w| w.dp_rank()) {
decode_request.set_data_parallel_rank(rank as i32);
}
Expand Down
4 changes: 2 additions & 2 deletions model_gateway/src/routers/grpc/epd_encode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use super::{
context::{ClientSelection, WorkerSelection},
multimodal::{assemble_tokenspeed, MultimodalIntermediate, PrecomputedMultimodalIntermediate},
proto_wrapper::{
cleanup_tokenspeed_items_encoder_shm, cleanup_tokenspeed_shm_handles,
cleanup_mm_shm_handles, cleanup_tokenspeed_items_encoder_shm,
collect_tokenspeed_multimodal_inputs_shm_handles, EncodeItemBootstrapInfo,
TokenSpeedMultimodalData, TokenSpeedMultimodalItem,
},
Expand Down Expand Up @@ -136,7 +136,7 @@ struct TokenSpeedShmCleanupGuard(Vec<common_proto::ShmHandle>);

impl Drop for TokenSpeedShmCleanupGuard {
fn drop(&mut self) {
cleanup_tokenspeed_shm_handles(&self.0);
cleanup_mm_shm_handles(&self.0);
}
}

Expand Down
9 changes: 7 additions & 2 deletions model_gateway/src/routers/grpc/multimodal/assemble.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ async fn assemble_multimodal_data_impl(
}
GrpcClient::Vllm(_) => {
ensure_image_only(&precomputed, "vLLM")?;
Ok(MultimodalData::Vllm(assemble_vllm(precomputed)))
Ok(MultimodalData::Vllm(assemble_vllm(precomputed, workers)))
}
GrpcClient::Trtllm(_) => {
ensure_image_only(&precomputed, "TRT-LLM")?;
Expand Down Expand Up @@ -168,7 +168,10 @@ fn assemble_sglang(intermediate: PrecomputedMultimodalIntermediate) -> SglangMul
}
}

fn assemble_vllm(intermediate: PrecomputedMultimodalIntermediate) -> VllmMultimodalData {
fn assemble_vllm(
intermediate: PrecomputedMultimodalIntermediate,
workers: Option<&WorkerSelection>,
) -> VllmMultimodalData {
let (pixel_values, pixel_values_shape) = serialize_encoder_input(&intermediate.preprocessed);
let model_specific_tensors = serialize_model_specific(intermediate.preprocessed.model_specific);
let mm_hashes = intermediate.images.iter().map(|f| f.hash.clone()).collect();
Expand All @@ -190,6 +193,8 @@ fn assemble_vllm(intermediate: PrecomputedMultimodalIntermediate) -> VllmMultimo
batched_keys,
flat_keys,
keep_on_cpu_keys: intermediate.keep_on_cpu_keys,
shm_enabled: resolve_mm_shm_enabled(workers, false),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid reusing one SHM handle for vLLM PD legs

In vLLM PD, execute_sequential_pd sends a cloned request to prefill and later reuses the original request as the decode request, so both legs carry the same mm_inputs. With SHM enabled here whenever the selected workers share /dev/shm, the prefill servicer reads and unlinks each segment, then the decode servicer receives the same ShmHandle and fails to open it before generation. Please either disable SHM for the multi-consumer vLLM PD path, create separate SHM payloads per leg, or keep the segment alive until both legs have consumed it.

Useful? React with 👍 / 👎.

shm_min_bytes: resolve_mm_shm_min_bytes(workers),
}
}

Expand Down
Loading
Loading