Skip to content

feat: new mutex GPU scheduling policy#2011

Merged
hami-robot[bot] merged 7 commits into
Project-HAMi:masterfrom
mesutoezdil:feat/mutex-policy
Jul 20, 2026
Merged

feat: new mutex GPU scheduling policy#2011
hami-robot[bot] merged 7 commits into
Project-HAMi:masterfrom
mesutoezdil:feat/mutex-policy

Conversation

@mesutoezdil

@mesutoezdil mesutoezdil commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Add a mutex GPU scheduling policy. A pod that sets hami.io/gpu-scheduler-policy: mutex is only placed on GPUs that have no other users (Used == 0).

Less() sorts busy GPUs first and idle GPUs to the tail, and Fit() skips any device with Used > 0 under the mutex policy, across all device backends.

Based on #2012 (Score-primary sort). This PR is rebased on top of #2012 and only adds the mutex code, so it does not repeat the sort fix.

Changes

  • pkg/util/types.go: add GPUSchedulerPolicyMutex.
  • pkg/scheduler/policy/gpu_policy.go: mutex branch in Less().
  • device backends: Fit() rejects used devices under the mutex policy.
  • tests for the mutex sort.

Fixes #2009
Part of #1889

Summary by CodeRabbit

  • New Features
    • Added a mutex GPU scheduling policy intended to allocate only idle devices.
    • Introduced NUMA bind-aware device ordering to preserve locality during selection.
  • Bug Fixes
    • Updated allocation behavior across supported device types to treat already-used candidates as exclusive allocation conflicts under mutex, reporting ExclusiveDeviceAllocateConflict.
  • Tests
    • Expanded unit tests to cover mutex rejection (including “used device” scenarios) and updated NUMA bind/policy sorting expectations.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Note

Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported.

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a6384574-b233-4ed7-9545-51ff14d5cb81

📥 Commits

Reviewing files that changed from the base of the PR and between 87064a2 and 4d3b33c.

📒 Files selected for processing (2)
  • pkg/device/vastai/device.go
  • pkg/device/vastai/device_test.go

📝 Walkthrough

Walkthrough

The PR adds the mutex GPU scheduling policy, rejects occupied devices across supported backends, and introduces NUMA-aware ordering with pod annotation propagation and coverage tests.

Changes

GPU scheduling behavior

Layer / File(s) Summary
Policy constants and device ordering
pkg/util/types.go, pkg/scheduler/policy/*
Adds the mutex policy and updates ordering for mutex, binpack, spread, and NUMA-bound scheduling.
NUMA binding propagation
pkg/scheduler/scheduler.go
Parses the NUMA-binding annotation and stores it in per-node device usage lists.
Mutex-aware backend allocation
pkg/device/{amd,ascend,awsneuron,biren,cambricon,enflame,hygon,iluvatar,kunlun,metax,mthreads,nvidia,vastai}/*
Rejects used devices under mutex scheduling and records ExclusiveDeviceAllocateConflict; Kunlun applies the rule through its fit predicate.
Backend and NUMA validation
pkg/device/*/*_test.go, pkg/scheduler/numa_sort_test.go
Tests occupied-device rejection, idle-device selection, NUMA affinity grouping, and score-based selection across NUMA nodes.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Pod
  participant Scheduler
  participant DeviceUsageList
  participant DeviceFit
  Pod->>Scheduler: provide mutex and NUMA annotations
  Scheduler->>DeviceUsageList: propagate NUMA binding
  DeviceFit->>DeviceUsageList: apply policy-aware ordering
  DeviceFit->>DeviceUsageList: inspect Used values
  DeviceFit-->>Pod: return allocation or ExclusiveDeviceAllocateConflict
Loading

Possibly related PRs

Suggested labels: enhancement

Suggested reviewers: dsfans2014, shouren, archlitchi

Poem

A rabbit hops through GPUs bright,
Busy cards are skipped from sight.
Mutex guards the idle lane,
NUMA keeps close friends in chain.
Tests sort onward, neat and proud.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning NUMA-bind and score-primary scheduler changes, plus their tests, are unrelated to the mutex GPU policy issue. Move the NUMA-related scheduler changes into a separate PR or link them to a matching issue.
Docstring Coverage ⚠️ Warning Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding a new mutex GPU scheduling policy.
Linked Issues check ✅ Passed The PR implements the mutex policy, adds the constant, updates sorting, and blocks used GPUs across device Fit paths as requested.
✨ 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.

@github-actions github-actions Bot added the kind/feature new function label Jul 3, 2026
@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.98496% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/device/kunlun/vdevice.go 78.57% 2 Missing and 1 partial ⚠️
pkg/device/vastai/device.go 87.50% 2 Missing and 1 partial ⚠️
pkg/scheduler/scheduler.go 77.77% 2 Missing ⚠️
Flag Coverage Δ
unittests 61.12% <93.98%> (+0.67%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pkg/device/amd/device.go 76.08% <100.00%> (+0.89%) ⬆️
pkg/device/ascend/device.go 83.60% <100.00%> (+0.19%) ⬆️
pkg/device/awsneuron/device.go 75.78% <100.00%> (+0.43%) ⬆️
pkg/device/biren/device.go 94.89% <100.00%> (+0.19%) ⬆️
pkg/device/cambricon/device.go 83.59% <100.00%> (+0.32%) ⬆️
pkg/device/enflame/device.go 55.50% <100.00%> (+0.70%) ⬆️
pkg/device/hygon/device.go 92.61% <100.00%> (+0.18%) ⬆️
pkg/device/iluvatar/device.go 57.99% <100.00%> (+0.98%) ⬆️
pkg/device/metax/device.go 75.87% <100.00%> (+0.62%) ⬆️
pkg/device/metax/sdevice.go 76.97% <100.00%> (+0.24%) ⬆️
... and 7 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

NUMA was the primary key in Less(), which broke binpack and spread when
devices span NUMA nodes. Score is now primary, NUMA is the tiebreaker.

Keep NUMA grouping when a pod requests numa-bind, so Fit can still
accumulate a same-NUMA run and NUMA affinity is not broken.

Fixes Project-HAMi#1806
Closes Project-HAMi#2010
Part of Project-HAMi#1889

Signed-off-by: mesutoezdil <mesudozdil@gmail.com>

@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: 2

🧹 Nitpick comments (4)
pkg/device/metax/sdevice_test.go (1)

1179-1207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the positive mutex path and assert failure results.

This only proves rejection. Add a used+idle case asserting the idle UUID is selected, and validate wantLen regardless of wantFit; otherwise a mutex implementation that rejects all devices, or returns partial allocations with false, still passes.

🤖 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 `@pkg/device/metax/sdevice_test.go` around lines 1179 - 1207, Extend the
mutex-policy tests around the existing “mutex policy rejects used device” case
to include both a used device and an idle device, asserting that the idle
device’s UUID is selected. Update assertions to always validate wantLen and
wantDevIDs regardless of wantFit, including failure cases, so implementations
that reject all devices or return partial allocations with false are caught.
pkg/scheduler/scheduler.go (1)

45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider moving the NumaBind annotation key to a shared package.

The scheduler now imports pkg/device/nvidia solely for the nvidia.NumaBind constant. Since NUMA binding is applied across all device backends (not just NVIDIA), defining the annotation key in a vendor-specific package creates an awkward coupling. Consider moving it to pkg/util or pkg/device to keep the scheduler vendor-agnostic.

Also applies to: 529-541, 559-559

🤖 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 `@pkg/scheduler/scheduler.go` at line 45, Move the NumaBind annotation constant
from the NVIDIA-specific package to a shared package such as pkg/device or
pkg/util, update its definition and all references including scheduler
NUMA-binding logic, remove the scheduler’s nvidia import, and adjust affected
tests or callers to use the shared symbol.
pkg/device/biren/device.go (1)

196-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Correct mutex-conflict guard, but duplicated across every backend.

The exact same 5-line block (compute isMutex, reject dev.Used > 0, increment common.ExclusiveDeviceAllocateConflict, log, continue) is copy-pasted identically across biren, iluvatar, metax, nvidia, vastai (and, per the PR description, all 13 device backends). Consider extracting a shared helper in pkg/device/common to avoid divergence risk as backends evolve independently.

♻️ Proposed helper extraction
// pkg/device/common/common.go
func RejectIfMutexConflict(reason map[string]int, pod *corev1.Pod, dev *device.DeviceUsage, isMutex bool, index int) bool {
	if isMutex && dev.Used > 0 {
		reason[ExclusiveDeviceAllocateConflict]++
		klog.V(5).InfoS(ExclusiveDeviceAllocateConflict, "pod", klog.KObj(pod), "device", dev.ID, "device index", index, "used", dev.Used)
		return true
	}
	return false
}

Call sites become:

-		if isMutex && dev.Used > 0 {
-			reason[common.ExclusiveDeviceAllocateConflict]++
-			klog.V(5).InfoS(common.ExclusiveDeviceAllocateConflict, "pod", klog.KObj(pod), "device", dev.ID, "device index", i, "used", dev.Used)
-			continue
-		}
+		if common.RejectIfMutexConflict(reason, pod, dev, isMutex, i) {
+			continue
+		}

Also applies to: 217-221

🤖 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 `@pkg/device/biren/device.go` at line 196, Extract the duplicated
mutex-conflict guard into a shared common helper, such as RejectIfMutexConflict,
accepting the reason map, pod, device usage, mutex flag, and index; have it
increment ExclusiveDeviceAllocateConflict, log the conflict, and return whether
allocation should be skipped. Replace the repeated isMutex/dev.Used checks and
associated logging across all device backends, including biren and the other
listed implementations, with this helper while preserving each backend’s
existing control flow.
pkg/device/amd/device_test.go (1)

529-553: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Consider adding a multi-device mutex selection test.

All new mutex test cases (across the 8 backend files) only verify rejection when the sole candidate device is busy. None verify that, given a mix of busy and idle devices under the mutex policy, Fit actually selects the idle one — which is the core scheduling behavior described in the PR. If this is already covered by gpu_policy_test.go's ordering tests, this can be skipped; otherwise, adding a device pair (one Used>0, one Used=0) here would close the gap.

🤖 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 `@pkg/device/amd/device_test.go` around lines 529 - 553, Add a mutex-policy
test covering mixed device availability in the relevant test table: include one
busy device (Used>0) and one idle device (Used=0), request a single AMD device,
and assert Fit succeeds while selecting only the idle device with the expected
ID and count. Reuse the existing test symbols and expectations such as wantFit,
wantLen, wantDevIDs, and wantReason, unless gpu_policy_test.go already verifies
this behavior.
🤖 Prompt for all review comments with 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.

Inline comments:
In `@pkg/device/awsneuron/device.go`:
- Line 372: The mutex policy is bypassed in the multi-device selection path. In
the request-handling logic near isMutex and graphSelect, filter out devices with
Used > 0 before calling graphSelect when mutex scheduling is enabled, while
preserving the existing behavior for non-mutex policies and single-device
requests.

In `@pkg/device/kunlun/vdevice.go`:
- Around line 233-240: The mutex-specific filter in Fit’s graghSelect path
currently causes rejected used VXPUs to be reported as NumaNotFit. Track whether
candidates were excluded by the mutex policy, return
ExclusiveDeviceAllocateConflict when that filter prevents allocation, and
preserve NumaNotFit for genuine topology failures; update
TestKunlunVDevices_Fit_Mutex to assert the new reason.

---

Nitpick comments:
In `@pkg/device/amd/device_test.go`:
- Around line 529-553: Add a mutex-policy test covering mixed device
availability in the relevant test table: include one busy device (Used>0) and
one idle device (Used=0), request a single AMD device, and assert Fit succeeds
while selecting only the idle device with the expected ID and count. Reuse the
existing test symbols and expectations such as wantFit, wantLen, wantDevIDs, and
wantReason, unless gpu_policy_test.go already verifies this behavior.

In `@pkg/device/biren/device.go`:
- Line 196: Extract the duplicated mutex-conflict guard into a shared common
helper, such as RejectIfMutexConflict, accepting the reason map, pod, device
usage, mutex flag, and index; have it increment ExclusiveDeviceAllocateConflict,
log the conflict, and return whether allocation should be skipped. Replace the
repeated isMutex/dev.Used checks and associated logging across all device
backends, including biren and the other listed implementations, with this helper
while preserving each backend’s existing control flow.

In `@pkg/device/metax/sdevice_test.go`:
- Around line 1179-1207: Extend the mutex-policy tests around the existing
“mutex policy rejects used device” case to include both a used device and an
idle device, asserting that the idle device’s UUID is selected. Update
assertions to always validate wantLen and wantDevIDs regardless of wantFit,
including failure cases, so implementations that reject all devices or return
partial allocations with false are caught.

In `@pkg/scheduler/scheduler.go`:
- Line 45: Move the NumaBind annotation constant from the NVIDIA-specific
package to a shared package such as pkg/device or pkg/util, update its
definition and all references including scheduler NUMA-binding logic, remove the
scheduler’s nvidia import, and adjust affected tests or callers to use the
shared symbol.
🪄 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 Plus

Run ID: 5d6d0642-b4ed-4aa2-94d5-d77cedcfb2cd

📥 Commits

Reviewing files that changed from the base of the PR and between 1dc4fb7 and 6509645.

📒 Files selected for processing (33)
  • pkg/device/amd/device.go
  • pkg/device/amd/device_test.go
  • pkg/device/ascend/device.go
  • pkg/device/ascend/device_test.go
  • pkg/device/awsneuron/device.go
  • pkg/device/awsneuron/device_test.go
  • pkg/device/biren/device.go
  • pkg/device/biren/device_test.go
  • pkg/device/cambricon/device.go
  • pkg/device/cambricon/device_test.go
  • pkg/device/enflame/device.go
  • pkg/device/enflame/device_test.go
  • pkg/device/hygon/device.go
  • pkg/device/hygon/device_test.go
  • pkg/device/iluvatar/device.go
  • pkg/device/iluvatar/device_test.go
  • pkg/device/kunlun/device_test.go
  • pkg/device/kunlun/vdevice.go
  • pkg/device/metax/device.go
  • pkg/device/metax/device_test.go
  • pkg/device/metax/sdevice.go
  • pkg/device/metax/sdevice_test.go
  • pkg/device/mthreads/device.go
  • pkg/device/mthreads/device_test.go
  • pkg/device/nvidia/device.go
  • pkg/device/nvidia/device_test.go
  • pkg/device/vastai/device.go
  • pkg/device/vastai/device_test.go
  • pkg/scheduler/numa_sort_test.go
  • pkg/scheduler/policy/gpu_policy.go
  • pkg/scheduler/policy/gpu_policy_test.go
  • pkg/scheduler/scheduler.go
  • pkg/util/types.go

Comment thread pkg/device/awsneuron/device.go
Comment thread pkg/device/kunlun/vdevice.go
Add GPUSchedulerPolicyMutex = "mutex" constant. When a pod sets
hami.io/gpu-scheduler-policy: mutex, the scheduler allocates only
GPUs with no existing users (Used == 0).

- Less(): mutex case sorts idle GPUs to tail for Fit() to pick first
- Fit(): isMutex filter rejects devices with Used > 0
- Applied to all 13 device backends

Closes Project-HAMi#2009
Part of Project-HAMi#1889

Signed-off-by: mesutoezdil <mesudozdil@gmail.com>

@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.

Caution

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

⚠️ Outside diff range comments (1)
pkg/scheduler/scheduler.go (1)

762-777: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Sort device vendors before locking to prevent deadlocks.

Iterating over device.GetDevices() yields vendors in a non-deterministic order because it returns a Go map. If multiple pods are scheduled concurrently on a node with multiple device vendors, acquiring locks in random orders can cause livelocks or deadlocks (e.g., Pod A locks Vendor 1 then waits for Vendor 2, while Pod B locks Vendor 2 then waits for Vendor 1).

Sort the vendor names to enforce a strict lock acquisition order, and release them in reverse order to ensure deterministic behavior.

🔒️ Proposed fix to acquire and release locks deterministically
-func (s *Scheduler) lockAllDevices(node *corev1.Node, pod *corev1.Pod) error {
-	for _, val := range device.GetDevices() {
-		if err := val.LockNode(node, pod); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func (s *Scheduler) releaseAllDevices(node *corev1.Node, pod *corev1.Pod) {
-	for _, val := range device.GetDevices() {
-		if err := val.ReleaseNodeLock(node, pod); err != nil {
-			klog.ErrorS(err, "Failed to release node lock", "node", node.Name, "pod", klog.KObj(pod))
-		}
-	}
-}
+func (s *Scheduler) lockAllDevices(node *corev1.Node, pod *corev1.Pod) error {
+	devs := device.GetDevices()
+	vendors := make([]string, 0, len(devs))
+	for vendor := range devs {
+		vendors = append(vendors, vendor)
+	}
+	sort.Strings(vendors)
+
+	for _, vendor := range vendors {
+		if err := devs[vendor].LockNode(node, pod); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (s *Scheduler) releaseAllDevices(node *corev1.Node, pod *corev1.Pod) {
+	devs := device.GetDevices()
+	vendors := make([]string, 0, len(devs))
+	for vendor := range devs {
+		vendors = append(vendors, vendor)
+	}
+	sort.Strings(vendors)
+
+	for i := len(vendors) - 1; i >= 0; i-- {
+		vendor := vendors[i]
+		if err := devs[vendor].ReleaseNodeLock(node, pod); err != nil {
+			klog.ErrorS(err, "Failed to release node lock", "node", node.Name, "pod", klog.KObj(pod))
+		}
+	}
+}

(Note: You may need to run goimports with the github.com/Project-HAMi/HAMi prefix to pull in the "sort" package if it is not already imported.)

🤖 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 `@pkg/scheduler/scheduler.go` around lines 762 - 777, Update lockAllDevices and
releaseAllDevices to collect device vendor names, sort them deterministically,
and access vendors in that order when locking. Release the same vendors in
reverse sorted order, preserving the existing error logging and return behavior.
🤖 Prompt for all review comments with 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.

Outside diff comments:
In `@pkg/scheduler/scheduler.go`:
- Around line 762-777: Update lockAllDevices and releaseAllDevices to collect
device vendor names, sort them deterministically, and access vendors in that
order when locking. Release the same vendors in reverse sorted order, preserving
the existing error logging and return behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fe6b03fd-0780-4945-949d-ec312b0c141e

📥 Commits

Reviewing files that changed from the base of the PR and between 275f87b and cf041fd.

📒 Files selected for processing (5)
  • pkg/device/ascend/device.go
  • pkg/device/ascend/device_test.go
  • pkg/device/nvidia/device.go
  • pkg/scheduler/scheduler.go
  • pkg/util/types.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • pkg/util/types.go
  • pkg/device/ascend/device_test.go
  • pkg/device/nvidia/device.go
  • pkg/device/ascend/device.go

…nder mutex

When the mutex policy rejects an otherwise-fitting device because it is
already in use, Fit() returned the generic NumaNotFit reason, making it
indistinguishable from a real topology mismatch.

Signed-off-by: mesutoezdil <mesudozdil@gmail.com>

@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: 1

🧹 Nitpick comments (1)
pkg/device/kunlun/vdevice.go (1)

244-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rename loop variable to avoid shadowing the method receiver.

The loop variable dev shadows the method receiver dev *KunlunVDevices. While this doesn't cause a functional bug due to scoping, shadowing the receiver can lead to confusion. Consider renaming it to d or deviceUsage.

♻️ Proposed refactor
-			for _, dev := range devices {
-				if dev.Used > 0 && FitVXPU(dev, request) {
+			for _, d := range devices {
+				if d.Used > 0 && FitVXPU(d, request) {
					reason[common.ExclusiveDeviceAllocateConflict]++
-					klog.V(5).InfoS(common.ExclusiveDeviceAllocateConflict, "pod", klog.KObj(pod), "device", dev.ID, "used", dev.Used)
+					klog.V(5).InfoS(common.ExclusiveDeviceAllocateConflict, "pod", klog.KObj(pod), "device", d.ID, "used", d.Used)
				}
			}
🤖 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 `@pkg/device/kunlun/vdevice.go` around lines 244 - 249, Rename the inner loop
variable in the device-conflict iteration to avoid shadowing the method
receiver, updating its Used, FitVXPU, and ID references consistently while
preserving the existing behavior.
🤖 Prompt for all review comments with 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.

Inline comments:
In `@pkg/device/kunlun/vdevice.go`:
- Line 253: Update the klog.V(5).InfoS call in the NUMA mismatch logging path to
remove the dangling "numa" key, leaving only complete key-value pairs while
preserving the existing log context.

---

Nitpick comments:
In `@pkg/device/kunlun/vdevice.go`:
- Around line 244-249: Rename the inner loop variable in the device-conflict
iteration to avoid shadowing the method receiver, updating its Used, FitVXPU,
and ID references consistently while preserving the existing behavior.
🪄 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 Plus

Run ID: 6c2bc4f1-40c5-4e2d-8ace-260f2dbffb06

📥 Commits

Reviewing files that changed from the base of the PR and between cf041fd and c3e2540.

📒 Files selected for processing (2)
  • pkg/device/kunlun/device_test.go
  • pkg/device/kunlun/vdevice.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/device/kunlun/device_test.go

Comment thread pkg/device/kunlun/vdevice.go Outdated
klog.InfoS expects key-value pairs; the trailing "numa" string had no
matching value, causing an odd argument count.

Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
@mesutoezdil

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@mesutoezdil

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai
coderabbitai Bot requested a review from Shouren July 17, 2026 20:04

@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: 1

🤖 Prompt for all review comments with 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.

Inline comments:
In `@pkg/device/vastai/device.go`:
- Line 234: Update the mutex-policy filtering around computeBestCombination to
precompute the AIC identifiers of all used dies, using each die’s
CustomInfo["AIC"], then reject every die whose AIC is in that occupied set.
Replace the current per-die Used check while preserving existing non-mutex
behavior and handling missing AIC values safely.
🪄 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 Plus

Run ID: 302405d7-c8a2-4c6e-9b7c-40716fbee507

📥 Commits

Reviewing files that changed from the base of the PR and between 125c8c6 and 87064a2.

📒 Files selected for processing (33)
  • pkg/device/amd/device.go
  • pkg/device/amd/device_test.go
  • pkg/device/ascend/device.go
  • pkg/device/ascend/device_test.go
  • pkg/device/awsneuron/device.go
  • pkg/device/awsneuron/device_test.go
  • pkg/device/biren/device.go
  • pkg/device/biren/device_test.go
  • pkg/device/cambricon/device.go
  • pkg/device/cambricon/device_test.go
  • pkg/device/enflame/device.go
  • pkg/device/enflame/device_test.go
  • pkg/device/hygon/device.go
  • pkg/device/hygon/device_test.go
  • pkg/device/iluvatar/device.go
  • pkg/device/iluvatar/device_test.go
  • pkg/device/kunlun/device_test.go
  • pkg/device/kunlun/vdevice.go
  • pkg/device/metax/device.go
  • pkg/device/metax/device_test.go
  • pkg/device/metax/sdevice.go
  • pkg/device/metax/sdevice_test.go
  • pkg/device/mthreads/device.go
  • pkg/device/mthreads/device_test.go
  • pkg/device/nvidia/device.go
  • pkg/device/nvidia/device_test.go
  • pkg/device/vastai/device.go
  • pkg/device/vastai/device_test.go
  • pkg/scheduler/numa_sort_test.go
  • pkg/scheduler/policy/gpu_policy.go
  • pkg/scheduler/policy/gpu_policy_test.go
  • pkg/scheduler/scheduler.go
  • pkg/util/types.go

Comment thread pkg/device/vastai/device.go
In die mode a physical card (AIC) is made of several dies. The mutex
guard only checked the current die's Used count, so an idle sibling die
on a physically occupied card could still be allocated, breaking the
whole-card exclusivity the mutex policy promises.

Precompute the set of occupied AICs and reject any idle die whose AIC is
already occupied. Non-mutex behavior is unchanged and missing AIC values
are handled safely. Add die-mode mutex tests.

Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

@mesutoezdil Thanks for addressing this. The physical-AIC exclusivity behavior and the two die-mode regression cases cover the previously identified mutex-policy gap. I’ll review the updated changes.


If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@mesutoezdil

Copy link
Copy Markdown
Contributor Author

/assign @archlitchi

@archlitchi archlitchi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@hami-robot hami-robot Bot added the lgtm label Jul 20, 2026
@hami-robot

hami-robot Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: archlitchi, mesutoezdil

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

The pull request process is described 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

@hami-robot hami-robot Bot added the approved label Jul 20, 2026
@hami-robot
hami-robot Bot merged commit 06d9b90 into Project-HAMi:master Jul 20, 2026
15 checks passed
klog.V(5).InfoS(common.CardTimeSlicingExhausted, "pod", klog.KObj(pod), "device", dev.ID, "count", dev.Count, "used", dev.Used)
continue
}
if isMutex && dev.Used > 0 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hi @mesutoezdil I'm a bit confused about the mutex policy.
Judging from the code, the mutex scheduling policy is implemented as restricted, not best-effort. If a card is already allocated via the mutex policy, can it still be shared with other Pods?
If so, why is restricted used here? And if it cannot be shared with other Pods, why use mutex instead of just requesting a full card?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hi @mesutoezdil I'm a bit confused about the mutex policy. Judging from the code, the mutex scheduling policy is implemented as restricted, not best-effort. If a card is already allocated via the mutex policy, can it still be shared with other Pods? If so, why is restricted used here? And if it cannot be shared with other Pods, why use mutex instead of just requesting a full card?

hi @DSFans2014 as implemented, it's a placement-time preference for the requesting pod, not a device-level lock, it matches #2009's literal spec ("only allocates GPUs with no existing users, skipped if already in use"), but that spec never covered protecting the device from later non-mutex pods.
So today: a mutex pod won't join a busy card, but a later non-mutex pod can still join a card a mutex pod already claimed.
If true mutual exclusion is the goal, that needs a persistent reservation marker on the device (checked by every pod's Fit(), not just mutex ones). If this is the ‘new/ideal’ architecture, let’s open a new issue. WDYT?

@DSFans2014 DSFans2014 Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mesutoezdil mesutoezdil Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@DSFans2014 sure, opened Project-HAMi/website#625 covering both pages

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: new mutex GPU scheduling policy

3 participants