Skip to content

Add backend object management - #30

Open
kpouget wants to merge 3 commits into
crc-org:mainfrom
kpouget:backend
Open

Add backend object management#30
kpouget wants to merge 3 commits into
crc-org:mainfrom
kpouget:backend

Conversation

@kpouget

@kpouget kpouget commented Apr 11, 2026

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • New Features

    • Added support for multiple virtual GPU backends per device, enabling more flexible GPU resource allocation and management.
  • Improvements

    • Enhanced backend lifecycle management with explicit initialization and cleanup operations.
    • Improved thread-safe state management for concurrent backend operations.

@openshift-ci

openshift-ci Bot commented Apr 11, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Apr 11, 2026

Copy link
Copy Markdown

Walkthrough

This refactoring transitions the backend from a single global initialization model to per-device, per-backend-instance management. It introduces lifecycle commands for initialize and cleanup, adds device context tracking with per-device backend instance maps, updates initialization to return device handles and backend IDs, and modifies graph compute to require these identifiers.

Changes

Cohort / File(s) Summary
Backend Dispatch Core
ggml/src/ggml-virtgpu/backend/backend-dispatched.cpp, ggml/src/ggml-virtgpu/backend/backend-dispatched.h
Replaced single global backend state with per-device/per-backend-instance tracking. Added apir_device_context struct with mutex-protected unordered_map for backend instances. Updated backend_dispatch_initialize signature to accept output parameters and create new backend instances. Introduced helper functions for device context and backend instance lifecycle management (get_device_context, ensure_device_context, cleanup_device_context, create_backend_instance, get_backend_instance, cleanup_backend_instance).
Backend Dispatch Handlers
ggml/src/ggml-virtgpu/backend/backend-dispatched-backend.cpp, ggml/src/ggml-virtgpu/backend/backend-dispatched.gen.h
Added new command handlers backend_backend_initialize and backend_backend_cleanup. Modified backend_backend_graph_compute to decode and route requests via device handle and backend ID instead of relying on implicit global state. Removed static cached async-initialization state and per-request validation of backend instances.
Command Protocol & Enums
ggml/src/ggml-virtgpu/backend/shared/apir_backend.gen.h, ggml/src/ggml-virtgpu/ggmlremoting_functions.yaml
Added APIR_COMMAND_TYPE_BACKEND_INITIALIZE (22) and APIR_COMMAND_TYPE_BACKEND_CLEANUP (24); renumbered APIR_COMMAND_TYPE_BACKEND_GRAPH_COMPUTE from 22 to 23. Updated dispatch table count to 25. Extended YAML remoting API to declare backend initialization and cleanup functions with device handle and backend ID parameters.
Backend Initialization
ggml/src/ggml-virtgpu/backend/backend.cpp
Modified apir_backend_initialize to explicitly invoke backend registration function, validate returned reg and device, and add null-pointer checks. Removed call to backend_dispatch_initialize and returns success/error status instead.
Remoting Frontend
ggml/src/ggml-virtgpu/virtgpu-forward-backend.cpp, ggml/src/ggml-virtgpu/virtgpu-forward.gen.h
Added new exported APIs apir_backend_initialize (encodes registration function, decodes device handle/backend ID) and apir_backend_cleanup (encodes identifiers, sends cleanup request). Updated apir_backend_graph_compute signature to accept and encode device handle and backend ID alongside the compute graph.
Remoting Backend Lifecycle
ggml/src/ggml-virtgpu/ggml-backend.cpp, ggml/src/ggml-virtgpu/ggml-backend-reg.cpp, ggml/src/ggml-virtgpu/ggml-remoting.h, ggml/src/ggml-virtgpu/backend/backend-virgl-apir.h
Added device_handle and backend_id fields to ggml_backend_remoting_device_context. Updated ggml_backend_remoting_device_init to explicitly call apir_backend_initialize with output parameters. Modified ggml_backend_remoting_free to call apir_backend_cleanup before deletion. Updated ggml_backend_remoting_graph_compute to pass device handle and backend ID to remote compute call. Removed exported global declaration extern ggml_backend_t bck.

Sequence Diagram

sequenceDiagram
    participant Client as Client (Frontend)
    participant RemotingBackend as Remoting Backend<br/>(ggml-backend.cpp)
    participant VirtGPU as VirtGPU<br/>(virtgpu-forward)
    participant DispatchBackend as Dispatch Backend<br/>(backend-dispatched)
    participant DeviceContext as Device Context<br/>& Backend Instance Map

    Note over Client,DeviceContext: Backend Initialization Flow
    Client->>RemotingBackend: ggml_backend_remoting_device_init(ctx)
    RemotingBackend->>VirtGPU: apir_backend_initialize(gpu, reg_fct, &device_handle, &backend_id)
    VirtGPU->>DispatchBackend: APIR_COMMAND_TYPE_BACKEND_INITIALIZE (encodes reg_fct)
    DispatchBackend->>DeviceContext: ensure_device_context(dev)
    DeviceContext-->>DispatchBackend: apir_device_context*
    DispatchBackend->>DeviceContext: create_backend_instance(dev)
    DeviceContext-->>DispatchBackend: backend_id, instance*
    DispatchBackend->>VirtGPU: response (device_handle, backend_id)
    VirtGPU-->>RemotingBackend: (device_handle, backend_id)
    RemotingBackend-->>Client: ggml_backend_t

    Note over Client,DeviceContext: Graph Compute Flow (with Device/Backend IDs)
    Client->>RemotingBackend: ggml_backend_remoting_graph_compute(backend, cgraph)
    RemotingBackend->>VirtGPU: apir_backend_graph_compute(gpu, device_handle, backend_id, cgraph)
    VirtGPU->>DispatchBackend: APIR_COMMAND_TYPE_BACKEND_GRAPH_COMPUTE<br/>(device_handle, backend_id, cgraph)
    DispatchBackend->>DeviceContext: get_backend_instance(dev, backend_id)
    DeviceContext-->>DispatchBackend: instance*
    DispatchBackend->>DispatchBackend: instance->bck->iface.graph_compute(cgraph)
    DispatchBackend->>VirtGPU: response
    VirtGPU-->>RemotingBackend: result
    RemotingBackend-->>Client: status

    Note over Client,DeviceContext: Backend Cleanup Flow
    Client->>RemotingBackend: ggml_backend_remoting_free(backend)
    RemotingBackend->>VirtGPU: apir_backend_cleanup(gpu, device_handle, backend_id)
    VirtGPU->>DispatchBackend: APIR_COMMAND_TYPE_BACKEND_CLEANUP<br/>(device_handle, backend_id)
    DispatchBackend->>DeviceContext: cleanup_backend_instance(dev, backend_id)
    DeviceContext->>DeviceContext: delete instance, ggml_backend_free(bck)
    DispatchBackend->>VirtGPU: response
    VirtGPU-->>RemotingBackend: void
    RemotingBackend-->>Client: success
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐇 Hops of Joy
Per-device backends bloom and grow,
No more one global in the snow!
Handles dance, IDs align,
Cleanup calls, then all is fine. ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning No pull request description was provided by the author; the description field is empty, violating the basic requirement to explain changes. Add a detailed pull request description explaining the motivation, changes, and testing performed. Reference the contributing guidelines if they require specific sections.
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add backend object management' clearly summarizes the main change, which involves refactoring backend lifecycle management from global state to per-device/per-backend instance management.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
ggml/src/ggml-virtgpu/ggml-backend.cpp (1)

66-83: ⚠️ Potential issue | 🔴 Critical

Keep backend-instance IDs off the shared device context.

dev->context is shared by every backend created from that device, but this path writes device_handle and backend_id into that shared object and then stores the same ctx on each ggml_backend. Initializing a second backend on the same device will overwrite the identifiers used by the first one, so later graph_compute() or free() calls can hit or destroy the wrong remote backend instance.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ggml/src/ggml-virtgpu/ggml-backend.cpp` around lines 66 - 83, The shared
dev->context (ggml_backend_remoting_device_context) is being used to store
per-backend identifiers (device_handle and backend_id), causing races/overwrites
when multiple ggml_backend instances are created for the same device; instead
allocate or clone a new per-backend context structure for each ggml_backend (do
not write into the shared dev->context), call apir_backend_initialize with that
per-backend context to populate its device_handle/backend_id, and set
ggml_backend::context to this new per-backend context; locate uses in
ggml_backend_remoting_device_context, apir_backend_initialize, ggml_backend_t
allocation, ggml_backend_remoting_interface, ggml_backend_remoting_guid and
ggml_backend_reg_dev_get to implement the per-backend context allocation and
initialization and ensure teardown/free uses the per-backend context.
🧹 Nitpick comments (2)
ggml/src/ggml-virtgpu/backend/backend-dispatched.h (1)

38-39: Type mismatch: next_backend_id is uintptr_t but API uses uint32_t.

next_backend_id is declared as uintptr_t (line 39), but backend_dispatch_initialize outputs uint32_t * out_backend_id (line 59), and get_backend_instance takes uintptr_t backend_id (line 56). This could cause truncation on 64-bit systems if many backends are created.

Consider using a consistent type throughout—either uint32_t (sufficient for practical use) or uintptr_t everywhere.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ggml/src/ggml-virtgpu/backend/backend-dispatched.h` around lines 38 - 39, The
current mix of uintptr_t and uint32_t for backend IDs can truncate on 64-bit
systems; make the ID type consistent by switching the internal storage and APIs
to uint32_t: change backend_instances key type from uintptr_t to uint32_t,
change next_backend_id from uintptr_t to uint32_t, and update
get_backend_instance's parameter to uint32_t (and any internal uses/casts) so
backend_dispatch_initialize's uint32_t *out_backend_id matches the internal
types everywhere.
ggml/src/ggml-virtgpu/virtgpu-forward-backend.cpp (1)

3-7: Misleading function name and potential dead code.

The function is named current_time_ms but returns nanoseconds (ts.tv_sec * 1000000000LL + ts.tv_nsec). If this is used elsewhere for timing, the calculation is correct for nanoseconds; otherwise, consider removing if unused.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ggml/src/ggml-virtgpu/virtgpu-forward-backend.cpp` around lines 3 - 7, The
function current_time_ms returns nanoseconds while its name implies
milliseconds; update the implementation or name to match intent: either rename
current_time_ms to current_time_ns (and adjust any callers that expect
nanoseconds) or change the calculation in current_time_ms to return milliseconds
by dividing the nanosecond value by 1'000'000; also consider switching
clock_gettime to CLOCK_MONOTONIC for elapsed timing if used for intervals and
remove the function entirely if it is unused. Ensure to update all references to
current_time_ms/current_time_ns accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ggml/src/ggml-virtgpu/backend/backend-dispatched.h`:
- Around line 29-47: The comment for the magic field in apir_backend_instance is
wrong—update the inline comment next to the backend instance's uint32_t magic to
match APIR_BACKEND_INSTANCE_MAGIC (0xCD4321BA); also verify the comment for
apir_device_context::magic matches APIR_DEVICE_EXTENSION_MAGIC (0xAB1234CD) so
the two struct comments correctly reference APIR_BACKEND_INSTANCE_MAGIC and
APIR_DEVICE_EXTENSION_MAGIC respectively (use the symbols apir_backend_instance,
apir_device_context, APIR_BACKEND_INSTANCE_MAGIC, APIR_DEVICE_EXTENSION_MAGIC to
locate and fix the comments).

In `@ggml/src/ggml-virtgpu/backend/backend.cpp`:
- Around line 107-117: The code currently always calls
ggml_backend_reg_dev_get(reg, 0) and stores that single dev for all backends
(via backend_reg_fct and dev), which pins every instance to device 0; instead,
propagate the selected device index through the new initialize flow and call
ggml_backend_reg_dev_get(reg, device_index) per backend instance (do not reuse
the single dev variable). Update the initialize/creation functions that call
backend_reg_fct/ggml_backend_reg_dev_get to accept/forward a device parameter
and obtain a device handle for each instance (refer to backend_reg_fct,
ggml_backend_reg_dev_get, and the dev variable) so backends requested for device
n use device n.

In `@ggml/src/ggml-virtgpu/ggmlremoting_functions.yaml`:
- Around line 143-148: The wire contract for initialize currently encodes three
values (int result, uintptr_t device_handle, uint32_t backend_id) but one
implementation decodes only two, causing shifted/corrupted ids; update the
decoder in the virtgpu frontend implementation of initialize to read the values
in the exact order and types declared by the contract (first read an int named
result, then a uintptr_t device_handle, then a uint32_t backend_id), handle
non-zero result appropriately (return/fail early), and ensure the
encoder/decoder ordering and types match exactly across implementations (result,
device_handle, backend_id).

In `@ggml/src/ggml-virtgpu/virtgpu-forward-backend.cpp`:
- Around line 114-128: The cleanup currently captures but ignores the
REMOTE_CALL return code in apir_backend_cleanup; update apir_backend_cleanup to
check the ApirForwardReturnCode ret after REMOTE_CALL (e.g., if ret != success)
and emit a diagnostic log including ret and context (device_handle, backend_id)
before calling remote_call_finish; reference apir_backend_cleanup, REMOTE_CALL,
ret, and remote_call_finish when making the change and use the existing logging
facility available on virtgpu (or stderr/fprintf if none) so cleanup failures
are visible for debugging.

---

Outside diff comments:
In `@ggml/src/ggml-virtgpu/ggml-backend.cpp`:
- Around line 66-83: The shared dev->context
(ggml_backend_remoting_device_context) is being used to store per-backend
identifiers (device_handle and backend_id), causing races/overwrites when
multiple ggml_backend instances are created for the same device; instead
allocate or clone a new per-backend context structure for each ggml_backend (do
not write into the shared dev->context), call apir_backend_initialize with that
per-backend context to populate its device_handle/backend_id, and set
ggml_backend::context to this new per-backend context; locate uses in
ggml_backend_remoting_device_context, apir_backend_initialize, ggml_backend_t
allocation, ggml_backend_remoting_interface, ggml_backend_remoting_guid and
ggml_backend_reg_dev_get to implement the per-backend context allocation and
initialization and ensure teardown/free uses the per-backend context.

---

Nitpick comments:
In `@ggml/src/ggml-virtgpu/backend/backend-dispatched.h`:
- Around line 38-39: The current mix of uintptr_t and uint32_t for backend IDs
can truncate on 64-bit systems; make the ID type consistent by switching the
internal storage and APIs to uint32_t: change backend_instances key type from
uintptr_t to uint32_t, change next_backend_id from uintptr_t to uint32_t, and
update get_backend_instance's parameter to uint32_t (and any internal
uses/casts) so backend_dispatch_initialize's uint32_t *out_backend_id matches
the internal types everywhere.

In `@ggml/src/ggml-virtgpu/virtgpu-forward-backend.cpp`:
- Around line 3-7: The function current_time_ms returns nanoseconds while its
name implies milliseconds; update the implementation or name to match intent:
either rename current_time_ms to current_time_ns (and adjust any callers that
expect nanoseconds) or change the calculation in current_time_ms to return
milliseconds by dividing the nanosecond value by 1'000'000; also consider
switching clock_gettime to CLOCK_MONOTONIC for elapsed timing if used for
intervals and remove the function entirely if it is unused. Ensure to update all
references to current_time_ms/current_time_ns accordingly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0917dd7b-cabe-46c4-ba2e-f8c90adc2388

📥 Commits

Reviewing files that changed from the base of the PR and between ffaafde and f793662.

📒 Files selected for processing (13)
  • ggml/src/ggml-virtgpu/backend/backend-dispatched-backend.cpp
  • ggml/src/ggml-virtgpu/backend/backend-dispatched.cpp
  • ggml/src/ggml-virtgpu/backend/backend-dispatched.gen.h
  • ggml/src/ggml-virtgpu/backend/backend-dispatched.h
  • ggml/src/ggml-virtgpu/backend/backend-virgl-apir.h
  • ggml/src/ggml-virtgpu/backend/backend.cpp
  • ggml/src/ggml-virtgpu/backend/shared/apir_backend.gen.h
  • ggml/src/ggml-virtgpu/ggml-backend-reg.cpp
  • ggml/src/ggml-virtgpu/ggml-backend.cpp
  • ggml/src/ggml-virtgpu/ggml-remoting.h
  • ggml/src/ggml-virtgpu/ggmlremoting_functions.yaml
  • ggml/src/ggml-virtgpu/virtgpu-forward-backend.cpp
  • ggml/src/ggml-virtgpu/virtgpu-forward.gen.h
💤 Files with no reviewable changes (1)
  • ggml/src/ggml-virtgpu/backend/backend-virgl-apir.h

Comment on lines +29 to +47
// Backend instance structure - one backend per instance
struct apir_backend_instance {
ggml_backend_t bck; // The actual backend
uint32_t magic; // For validation: 0xAB1234CD
};

// Device context structure - can have multiple backend instances
struct apir_device_context {
std::mutex backends_mutex;
std::unordered_map<uintptr_t, apir_backend_instance *> backend_instances;
uintptr_t next_backend_id;

bool async_backend; // Whether the backend supports async operations

uint32_t magic; // For validation: 0xAB1234CD
};

#define APIR_DEVICE_EXTENSION_MAGIC 0xAB1234CD
#define APIR_BACKEND_INSTANCE_MAGIC 0xCD4321BA

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify async_backend naming consistency
rg -n 'async_backend' --type=cpp ggml/src/ggml-virtgpu/backend/

Repository: crc-org/llama.cpp

Length of output: 422


Magic number comment is incorrect.

Line 32 comment says 0xAB1234CD but APIR_BACKEND_INSTANCE_MAGIC is defined as 0xCD4321BA on line 47. The comment should reference the correct magic value:

🔧 Fix comment to match actual magic value
 // Backend instance structure - one backend per instance
 struct apir_backend_instance {
     ggml_backend_t bck;    // The actual backend
-    uint32_t       magic;  // For validation: 0xAB1234CD
+    uint32_t       magic;  // For validation: 0xCD4321BA
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Backend instance structure - one backend per instance
struct apir_backend_instance {
ggml_backend_t bck; // The actual backend
uint32_t magic; // For validation: 0xAB1234CD
};
// Device context structure - can have multiple backend instances
struct apir_device_context {
std::mutex backends_mutex;
std::unordered_map<uintptr_t, apir_backend_instance *> backend_instances;
uintptr_t next_backend_id;
bool async_backend; // Whether the backend supports async operations
uint32_t magic; // For validation: 0xAB1234CD
};
#define APIR_DEVICE_EXTENSION_MAGIC 0xAB1234CD
#define APIR_BACKEND_INSTANCE_MAGIC 0xCD4321BA
// Backend instance structure - one backend per instance
struct apir_backend_instance {
ggml_backend_t bck; // The actual backend
uint32_t magic; // For validation: 0xCD4321BA
};
// Device context structure - can have multiple backend instances
struct apir_device_context {
std::mutex backends_mutex;
std::unordered_map<uintptr_t, apir_backend_instance *> backend_instances;
uintptr_t next_backend_id;
bool async_backend; // Whether the backend supports async operations
uint32_t magic; // For validation: 0xAB1234CD
};
`#define` APIR_DEVICE_EXTENSION_MAGIC 0xAB1234CD
`#define` APIR_BACKEND_INSTANCE_MAGIC 0xCD4321BA
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ggml/src/ggml-virtgpu/backend/backend-dispatched.h` around lines 29 - 47, The
comment for the magic field in apir_backend_instance is wrong—update the inline
comment next to the backend instance's uint32_t magic to match
APIR_BACKEND_INSTANCE_MAGIC (0xCD4321BA); also verify the comment for
apir_device_context::magic matches APIR_DEVICE_EXTENSION_MAGIC (0xAB1234CD) so
the two struct comments correctly reference APIR_BACKEND_INSTANCE_MAGIC and
APIR_DEVICE_EXTENSION_MAGIC respectively (use the symbols apir_backend_instance,
apir_device_context, APIR_BACKEND_INSTANCE_MAGIC, APIR_DEVICE_EXTENSION_MAGIC to
locate and fix the comments).

Comment on lines +107 to +117
reg = backend_reg_fct();
if (reg == NULL) {
return (ApirLoadLibraryReturnCode) (APIR_LOAD_LIBRARY_INIT_BASE_INDEX +
APIR_BACKEND_INITIALIZE_BACKEND_INIT_FAILED);
}

dev = ggml_backend_reg_dev_get(reg, 0);
if (dev == NULL) {
return (ApirLoadLibraryReturnCode) (APIR_LOAD_LIBRARY_INIT_BASE_INDEX +
APIR_BACKEND_INITIALIZE_BACKEND_INIT_FAILED);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Don't pin every initialized backend to device 0.

This stores a single dev = ggml_backend_reg_dev_get(reg, 0) for all later backend-instance creation. The frontend init path is device-specific, but nothing in the new initialize flow carries that selection through, so a backend requested for device n > 0 will still be instantiated on the first remote device.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ggml/src/ggml-virtgpu/backend/backend.cpp` around lines 107 - 117, The code
currently always calls ggml_backend_reg_dev_get(reg, 0) and stores that single
dev for all backends (via backend_reg_fct and dev), which pins every instance to
device 0; instead, propagate the selected device index through the new
initialize flow and call ggml_backend_reg_dev_get(reg, device_index) per backend
instance (do not reuse the single dev variable). Update the initialize/creation
functions that call backend_reg_fct/ggml_backend_reg_dev_get to accept/forward a
device parameter and obtain a device handle for each instance (refer to
backend_reg_fct, ggml_backend_reg_dev_get, and the dev variable) so backends
requested for device n use device n.

Comment on lines +143 to +148
initialize:
frontend_return: "int"
frontend_extra_params:
- "void *ggml_backend_reg_fct_p"
- "uintptr_t* out_device_handle"
- "uint32_t* out_backend_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.

⚠️ Potential issue | 🔴 Critical

The new initialize wire contract is out of sync with its implementations.

This API now declares an int return plus two output parameters, but the current backend/frontend pair disagree on the success payload: ggml/src/ggml-virtgpu/backend/backend-dispatched-backend.cpp:12-49 encodes result, device_handle, backend_id, while ggml/src/ggml-virtgpu/virtgpu-forward-backend.cpp:9-54 decodes only device_handle and backend_id. On the first successful init, both returned identifiers will be shifted/corrupted. Make both sides agree on one exact response shape.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ggml/src/ggml-virtgpu/ggmlremoting_functions.yaml` around lines 143 - 148,
The wire contract for initialize currently encodes three values (int result,
uintptr_t device_handle, uint32_t backend_id) but one implementation decodes
only two, causing shifted/corrupted ids; update the decoder in the virtgpu
frontend implementation of initialize to read the values in the exact order and
types declared by the contract (first read an int named result, then a uintptr_t
device_handle, then a uint32_t backend_id), handle non-zero result appropriately
(return/fail early), and ensure the encoder/decoder ordering and types match
exactly across implementations (result, device_handle, backend_id).

Comment on lines +114 to +128
void apir_backend_cleanup(virtgpu * gpu, uintptr_t device_handle, uint32_t backend_id) {
apir_encoder * encoder;
apir_decoder * decoder;
ApirForwardReturnCode ret;

REMOTE_CALL_PREPARE(gpu, encoder, APIR_COMMAND_TYPE_BACKEND_CLEANUP);

// Send device handle and backend ID separately
apir_encode_uintptr_t(encoder, &device_handle);
apir_encode_uint32_t(encoder, &backend_id);

REMOTE_CALL(gpu, encoder, decoder, ret);

remote_call_finish(gpu, encoder, decoder);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Unused ret variable - cleanup errors are silently ignored.

The ret return code from REMOTE_CALL is captured but never used. While cleanup functions often tolerate failures (since there's little recourse), logging the error would aid debugging shutdown issues.

🔧 Suggested fix to log cleanup failures
     REMOTE_CALL(gpu, encoder, decoder, ret);
 
+    if (ret != 0) {
+        GGML_LOG_WARN(GGML_VIRTGPU "%s: Backend cleanup returned: %d\n", __func__, ret);
+    }
+
     remote_call_finish(gpu, encoder, decoder);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void apir_backend_cleanup(virtgpu * gpu, uintptr_t device_handle, uint32_t backend_id) {
apir_encoder * encoder;
apir_decoder * decoder;
ApirForwardReturnCode ret;
REMOTE_CALL_PREPARE(gpu, encoder, APIR_COMMAND_TYPE_BACKEND_CLEANUP);
// Send device handle and backend ID separately
apir_encode_uintptr_t(encoder, &device_handle);
apir_encode_uint32_t(encoder, &backend_id);
REMOTE_CALL(gpu, encoder, decoder, ret);
remote_call_finish(gpu, encoder, decoder);
}
void apir_backend_cleanup(virtgpu * gpu, uintptr_t device_handle, uint32_t backend_id) {
apir_encoder * encoder;
apir_decoder * decoder;
ApirForwardReturnCode ret;
REMOTE_CALL_PREPARE(gpu, encoder, APIR_COMMAND_TYPE_BACKEND_CLEANUP);
// Send device handle and backend ID separately
apir_encode_uintptr_t(encoder, &device_handle);
apir_encode_uint32_t(encoder, &backend_id);
REMOTE_CALL(gpu, encoder, decoder, ret);
if (ret != 0) {
GGML_LOG_WARN(GGML_VIRTGPU "%s: Backend cleanup returned: %d\n", __func__, ret);
}
remote_call_finish(gpu, encoder, decoder);
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ggml/src/ggml-virtgpu/virtgpu-forward-backend.cpp` around lines 114 - 128,
The cleanup currently captures but ignores the REMOTE_CALL return code in
apir_backend_cleanup; update apir_backend_cleanup to check the
ApirForwardReturnCode ret after REMOTE_CALL (e.g., if ret != success) and emit a
diagnostic log including ret and context (device_handle, backend_id) before
calling remote_call_finish; reference apir_backend_cleanup, REMOTE_CALL, ret,
and remote_call_finish when making the change and use the existing logging
facility available on virtgpu (or stderr/fprintf if none) so cleanup failures
are visible for debugging.

@kpouget

kpouget commented Apr 11, 2026

Copy link
Copy Markdown
Collaborator Author

/test topsail remoting_mac
/cluster mac

@psap-forge-bot

Copy link
Copy Markdown

🔴 Test of 'mac_ai test prepare_ci' failed after 00 hours 19 minutes 33 seconds. 🔴

• Link to the test results.

• No reports index generated...

Test configuration:

PR_POSITIONAL_ARGS: topsail
PR_POSITIONAL_ARG_0: topsail
PR_POSITIONAL_ARG_1: remoting_mac

Failure indicator: Empty. (See run.log)

@kpouget

kpouget commented Apr 12, 2026

Copy link
Copy Markdown
Collaborator Author

/test topsail remoting_mac
/cluster mac
/skip pre_cleanup_ci

@psap-forge-bot

Copy link
Copy Markdown

🔴 Test of 'mac_ai test prepare_ci' failed after 00 hours 09 minutes 09 seconds. 🔴

• Link to the test results.

• No reports index generated...

Test configuration:

PR_POSITIONAL_ARGS: topsail
PR_POSITIONAL_ARG_0: topsail
PR_POSITIONAL_ARG_1: remoting_mac

Failure indicator: Empty. (See run.log)

@openshift-ci

openshift-ci Bot commented Apr 12, 2026

Copy link
Copy Markdown

@kpouget: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/topsail f793662 link true /test topsail

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant