diff --git a/Makefile b/Makefile index 29421a90a..c4b6e49d3 100644 --- a/Makefile +++ b/Makefile @@ -807,6 +807,22 @@ verify-rocm-smoke: ## ROCm smoke: build, link and generate on the GPU, asserting verify-rocm: verify-versions verify-kernel-dtype-keys verify-kernel-port-dispatch verify-llama-compat verify-rocm-overlay verify-python-tooling verify-fmt verify-clippy-rocm verify-rocm-smoke verify-test-rocm ## Run the ROCm gate locally on an AMD host (issue #1811) @echo "$(GREEN)[verify-rocm] OK$(RESET)" +# The gate for a shared gfx1151 host (issue #2244). Build everything the gate +# runs without the guard lock, then run `verify-rocm` under one `--hold` of +# the host-wide lock: the cargo rebuilds inside are cache hits, no +# other unit's guard can find an idle window in the middle of the gate, and an +# unguarded `verify-rocm` no longer invalidates everyone else's windows. The +# test, smoke and clippy builds must stay outside the hold, or the lock would be held through +# the compile. `verify-rocm` itself is unchanged for hosts with one tenant. +.PHONY: verify-rocm-held +verify-rocm-held: ## ROCm gate for a shared host: build unlocked, then run verify-rocm under one rocm_gpu_guard.sh --hold (issue #2244) + @echo "$(CYAN)[verify-rocm-held] building the gate's artifacts without the lock...$(RESET)" + $(CARGO) test --workspace --profile test-fast $(ROCM_JOBS) --features rocm --no-run + $(CARGO) build --release $(ROCM_JOBS) --features rocm + $(CARGO) clippy --workspace --all-targets $(ROCM_JOBS) --features rocm -- -D warnings + @echo "$(CYAN)[verify-rocm-held] running verify-rocm under one guard lock hold...$(RESET)" + @bash scripts/rocm_gpu_guard.sh --hold -- $(MAKE) verify-rocm + .PHONY: verify-versions verify-versions: ## Assert every version-tracking workspace crate carries the root `mlxcel` version @echo "$(CYAN)[verify] workspace crate versions...$(RESET)" diff --git a/docs/benchmarks.md b/docs/benchmarks.md index 551d328f4..35ecd2458 100644 --- a/docs/benchmarks.md +++ b/docs/benchmarks.md @@ -240,6 +240,17 @@ published page cites. Guards on one host take turns: each holds an `flock` on `ROCM_GPU_GUARD_LOCK` (default `/tmp/mlxcel-rocm-gpu-guard.lock`) from before its first idle wait until it exits, so two guards started together run one after the other instead of rejecting each other's command as contention (issue #2146), and parallel jobs no longer need different `--idle-secs` values. Any user's guard can share the lock file: it is created world-writable if missing and opened read-only. `--max-wait` is one budget for the lock wait and the idle waits together, counted from guard start without the time the command runs; a guard that runs out of it while waiting for the lock exits 75 without running the command. The command runs without the lock descriptor and with `ROCM_GPU_GUARD_LOCK_HELD` set, so a guard nested inside it (one whose ancestor is that guard) skips the lock rather than deadlocking; guards nested in the same outer guard are not serialized among themselves, and nesting through `sudo` needs `--preserve-env=ROCM_GPU_GUARD_LOCK_HELD`. The guard needs `flock` (util-linux) and exits 2 without it. Any local user can open the default lock file and so hold it, or pre-create it unreadable; on a host shared with untrusted users, point `ROCM_GPU_GUARD_LOCK` at a path only the benchmarking users can reach. +#### Sessions on a shared host (issue #2244) + +A guard takes the lock for one command, so a gate or a measurement that guards each run separately queues again for every run, and other units' compiles and test runs fill the gaps between them. Hold the lock once for the whole session instead: `scripts/rocm_gpu_guard.sh --hold [--max-wait SECS] -- COMMAND` takes the lock through the same wait (exit 75 when `--max-wait` runs out), does no idle wait and no monitoring, exports `ROCM_GPU_GUARD_LOCK_HELD`, runs COMMAND in the foreground, exits with its status and releases the lock on exit, INT and TERM. Guards started inside COMMAND skip the lock but keep their idle wait and monitor, so each run is still checked. `--idle-secs`, `--max-attempts` and `--log` are rejected with `--hold` (exit 2); under an outer guard `--hold` just runs COMMAND. + +- Build first, without the lock: a hold taken before a compile makes everyone else wait through it, and the compiler would also keep the inner guards from finding an idle window. +- Gate: `make verify-rocm-held` builds the test, release and clippy artifacts unlocked, then runs `make verify-rocm` under one `--hold`, so its rebuilds are cache hits. Do not run a bare `make verify-rocm` on a shared host: it runs GPU tests with no lock and invalidates other guards' idle windows (#2192's parity guard was rejected 28 times that way). +- Measurement: wrap the whole session in one hold and keep a guard on each run inside it, for example `scripts/rocm_gpu_guard.sh --hold -- bash -c 'for i in 1 2 3; do scripts/rocm_gpu_guard.sh --log run$i.log -- ./scripts/bench_decode.sh ...; done'`. +- `scripts/rocm_gpu_guard.sh --status` prints whether the lock is held and by whom, read from `ROCM_GPU_GUARD_LOCK.holder` (pid, start time, mode, command line), which every holder writes after it takes the lock and removes on exit; a file naming a pid that is gone is reported as stale. Exit 0 when the lock is free, 1 when it is held. A lock taken by plain `flock` has no holder file. + +The lock is not a queue: `flock` does not serve waiters in order. If `--hold` sessions still starve, file a FIFO ticket queue with the measurements. + ### ROCm per-kernel decode profile (issue #2061) `scripts/rocm_decode_profile.sh` answers where ROCm decode time goes, per diff --git a/scripts/rocm_gpu_guard.sh b/scripts/rocm_gpu_guard.sh index fe1ad1303..ac90ce9c9 100755 --- a/scripts/rocm_gpu_guard.sh +++ b/scripts/rocm_gpu_guard.sh @@ -4,6 +4,8 @@ # Usage: # scripts/rocm_gpu_guard.sh [--idle-secs N] [--max-attempts N] [--max-wait SECS] # [--log FILE] -- COMMAND [ARGS...] +# scripts/rocm_gpu_guard.sh --hold [--max-wait SECS] -- COMMAND [ARGS...] +# scripts/rocm_gpu_guard.sh --status # # A benchmark that shares the GPU with another process, or the UMA memory bus # with a compiler, is not a measurement. This is the guard the gfx1151 baseline @@ -39,6 +41,21 @@ # and the monitor and exit 130 / 143. The three numeric options take plain # non-negative integers. # +# --hold takes the lock and nothing else, for a whole session (#2244): it waits +# for the lock through the same --max-wait budget (exit 75 when that runs out), +# does no idle wait and no monitoring, exports ROCM_GPU_GUARD_LOCK_HELD= +# and runs COMMAND in the foreground with COMMAND's status as its own. Guards +# started inside COMMAND skip the lock but keep their idle wait and monitor, so +# a gate or a multi-run measurement queues for the lock once instead of once +# per run, and other units cannot slip between the runs. Under an outer guard +# --hold just runs COMMAND. --idle-secs, --max-attempts and --log are refused +# with --hold (exit 2). INT and TERM stop COMMAND and exit 130 / 143. +# +# Whoever holds the lock writes ROCM_GPU_GUARD_LOCK.holder (pid, start time, +# mode, command line) and removes it on exit. --status prints whether the lock +# is held and, from that file, by whom (a file naming a pid that is gone is +# reported as stale); exit 0 when the lock is free, 1 when it is held. +# # The sampling interval is one second: a GPU job shorter than that can in # principle be missed. The compiler list matches /proc//comm exactly. @@ -50,6 +67,10 @@ MAX_WAIT=0 LOG="" KFD_PROC_DIR="${ROCM_GPU_GUARD_KFD_DIR:-/sys/class/kfd/kfd/proc}" LOCK="${ROCM_GPU_GUARD_LOCK:-/tmp/mlxcel-rocm-gpu-guard.lock}" +HOLDER="$LOCK.holder" +HOLD=0 +STATUS=0 +GIVEN="" # Build tools whose memory traffic or CPU load would distort a UMA measurement. # Driver processes (make, cmake, ninja, build scripts) are left out: they are # idle while their compiler children, which are listed, do the work. @@ -57,13 +78,60 @@ LOCK="${ROCM_GPU_GUARD_LOCK:-/tmp/mlxcel-rocm-gpu-guard.lock}" COMPILER_RE="${ROCM_GPU_GUARD_COMPILER_RE:-^(cargo|rustc|clang|clang\+\+|clang-[0-9]+|hipcc|nvcc|cc1|cc1plus|ld|ld\.lld|ld\.gold|ld\.bfd|lld|collect2)$}" usage() { - sed -n '2,43p' "$0" | sed 's/^# \{0,1\}//' + sed -n '2,60p' "$0" | sed 's/^# \{0,1\}//' } +# --status: say whether the lock is held and by whom. The holder file is +# written by whoever took the lock, after it took it, so it can lag a fresh +# acquire by a moment and can outlive a holder that was killed with SIGKILL. +guard_status() { + command -v flock >/dev/null 2>&1 || { echo "rocm_gpu_guard: flock not found (util-linux)" >&2; exit 2; } + if [[ ! -e "$LOCK" ]] || flock -n "$LOCK" true 2>/dev/null; then + echo "guard lock $LOCK: free" + exit 0 + fi + echo "guard lock $LOCK: held" + if [[ -r "$HOLDER" ]]; then + local pid="" line state="running" + while IFS= read -r line; do + [[ "$line" == pid=* ]] && pid="${line#pid=}" + done <"$HOLDER" + if ! [[ "$pid" =~ ^[1-9][0-9]*$ && -e "/proc/$pid" ]]; then + state="stale (pid ${pid:-unknown} is gone; the lock is held by something else)" + fi + echo "holder file $HOLDER: $state" + sed 's/^/ /' "$HOLDER" + else + echo "no holder file $HOLDER (the lock was taken by something other than this script)" + fi + exit 1 +} + +# Record who holds the lock, best effort: a file left by another user in a +# sticky directory cannot be replaced, and the lock does not depend on it. +write_holder() { + local mode="$1"; shift + [[ -e "$HOLDER" ]] || { (umask 000; : >>"$HOLDER") 2>/dev/null || true; } + { + printf 'pid=%s\nstart=%s\nmode=%s\ncmd=' "$$" "$(date '+%Y-%m-%dT%H:%M:%S%z')" "$mode" + printf '%q ' "$@"; printf '\n' + } >"$HOLDER" 2>/dev/null || true + HOLDER_WRITTEN=1 +} +HOLDER_WRITTEN=0 +# Remove the holder file only if it still names this process. +clear_holder() { + (( HOLDER_WRITTEN )) || return 0 + if grep -qx "pid=$$" "$HOLDER" 2>/dev/null; then rm -f "$HOLDER" 2>/dev/null || : >"$HOLDER" 2>/dev/null || true; fi + return 0 +} +trap clear_holder EXIT + while [[ $# -gt 0 ]]; do case "$1" in --idle-secs|--max-attempts|--max-wait|--log) [[ $# -ge 2 ]] || { echo "rocm_gpu_guard: $1 needs a value" >&2; exit 2; } + GIVEN="$GIVEN $1" case "$1" in --idle-secs) IDLE_SECS="$2" ;; --max-attempts) MAX_ATTEMPTS="$2" ;; @@ -71,6 +139,8 @@ while [[ $# -gt 0 ]]; do --log) LOG="$2" ;; esac shift 2 ;; + --hold) HOLD=1; shift ;; + --status) STATUS=1; shift ;; -h|--help) usage; exit 0 ;; --) shift; break ;; *) echo "rocm_gpu_guard: unknown option $1" >&2; usage >&2; exit 2 ;; @@ -85,8 +155,19 @@ for opt in IDLE_SECS MAX_ATTEMPTS MAX_WAIT; do done # Force base 10 so a value such as 08 is not read as octal. IDLE_SECS=$((10#$IDLE_SECS)); MAX_ATTEMPTS=$((10#$MAX_ATTEMPTS)); MAX_WAIT=$((10#$MAX_WAIT)) +if (( STATUS )); then + if (( HOLD )) || [[ -n "$GIVEN" || $# -gt 0 ]]; then + echo "rocm_gpu_guard: --status takes no other option and no command" >&2 + exit 2 + fi + guard_status +fi +if (( HOLD )) && [[ -n "$GIVEN" && "$GIVEN" =~ (--idle-secs|--max-attempts|--log) ]]; then + echo "rocm_gpu_guard: --hold takes only --max-wait (no idle wait, attempts or log)" >&2 + exit 2 +fi [[ $# -gt 0 ]] || { echo "rocm_gpu_guard: no command given" >&2; usage >&2; exit 2; } -[[ -d "$KFD_PROC_DIR" ]] || { echo "rocm_gpu_guard: $KFD_PROC_DIR not found (no ROCm KFD driver?)" >&2; exit 2; } +(( HOLD )) || [[ -d "$KFD_PROC_DIR" ]] || { echo "rocm_gpu_guard: $KFD_PROC_DIR not found (no ROCm KFD driver?)" >&2; exit 2; } # --max-wait counts from here. run_secs is the time COMMAND has run, which is # not waiting. SECONDS=0 @@ -255,6 +336,27 @@ acquire_lock() { note "acquired guard lock $LOCK after $((SECONDS - start))s" } +if (( HOLD )); then + if [[ -n "$OUTER_GUARD" ]]; then + note "lock held by outer guard ${OUTER_GUARD}: --hold runs the command without taking $LOCK" + exec "$@" + fi + acquire_lock || exit 75 + write_holder hold "$@" + export ROCM_GPU_GUARD_LOCK_HELD=$$ + note "holding the lock for: $*" + # The lock descriptor stays out of COMMAND, so a daemon it leaves behind + # cannot keep the lock. The explicit stdin redirect keeps COMMAND's stdin + # (a background job would otherwise read /dev/null); it runs in the + # background only so INT and TERM can stop it while this shell waits. + "$@" {LOCK_FD}<&- <&0 & + cmd_pid=$! + rc=0 + wait "$cmd_pid" || rc=$? + note "held command exited ${rc}: releasing $LOCK" + exit "$rc" +fi + if [[ -n "$OUTER_GUARD" ]]; then note "lock held by outer guard ${OUTER_GUARD}: not taking $LOCK" # No lock to hold here; a placeholder descriptor keeps the closing @@ -267,6 +369,7 @@ else note "gave up: $((MAX_WAIT - (SECONDS - run_secs)))s of --max-wait left after the lock wait, shorter than ${IDLE_SECS}s of idle" exit 75 fi + write_holder guard "$@" export ROCM_GPU_GUARD_LOCK_HELD=$$ fi diff --git a/tests/test_rocm_decode_profile.py b/tests/test_rocm_decode_profile.py index 009b26dd0..7b86b5cd0 100644 --- a/tests/test_rocm_decode_profile.py +++ b/tests/test_rocm_decode_profile.py @@ -313,6 +313,150 @@ def test_a_daemon_left_by_the_command_does_not_keep_the_lock(self): finally: os.kill(daemon, 9) + # --hold and --status (issue #2244) + + def start_hold(self, kfd, *cmd, max_wait=None): + args = ["--hold"] + (["--max-wait", str(max_wait)] if max_wait else []) + ["--", *cmd] + p = subprocess.Popen(["bash", str(GUARD), *args], env=guard_env(kfd), + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) + self.addCleanup(p.communicate) + self.addCleanup(p.kill) + for _ in range(100): + if not lock_is_free(_TEST_LOCK): + return p + time.sleep(0.05) + self.fail("the --hold guard never took the lock") + + def test_hold_runs_nested_guards_without_taking_the_lock(self): + with tempfile.TemporaryDirectory() as kfd, tempfile.TemporaryDirectory() as out: + log = pathlib.Path(out) / "inner.log" + inner = (f"bash {GUARD} --idle-secs 1 --max-wait 10 --log {log} -- true; " + f"bash {GUARD} --idle-secs 1 --max-wait 10 --log {log} -- bash -c 'exit 4'; " + 'test "$ROCM_GPU_GUARD_LOCK_HELD" = "$PPID" || exit 9; ' + f"flock -n {_TEST_LOCK} true && exit 8; exit 3") + r = run_guard(kfd, "--hold", "--", "bash", "-c", inner) + # The inner guards ran their own idle wait and monitor (a CLEAN + # attempt each), took no lock, and the lock stayed held throughout + # (exit 8 would mean it was free); the command's status comes back. + self.assertEqual(r.returncode, 3, r.stderr) + self.assertEqual(r.stderr.count("lock held by outer guard"), 2, r.stderr) + self.assertNotIn("waiting for guard lock", r.stderr) + self.assertEqual(log.read_text().count("CLEAN, exit"), 2, log.read_text()) + self.assertIn("exit 4", log.read_text()) + self.assertTrue(lock_is_free(_TEST_LOCK)) + + def test_hold_does_no_idle_wait_even_with_a_busy_gpu(self): + with tempfile.TemporaryDirectory() as kfd: + os.mkdir(f"{kfd}/1") + r = run_guard(kfd, "--hold", "--", "true") + self.assertEqual(r.returncode, 0, r.stderr) + self.assertNotIn("waiting for", r.stderr.replace("waiting for guard lock", "")) + self.assertNotIn("attempt", r.stderr) + + def test_a_second_hold_waits_for_the_first_and_exits_75_on_max_wait(self): + with tempfile.TemporaryDirectory() as kfd, tempfile.TemporaryDirectory() as out: + first = self.start_hold(kfd, "sleep", "8") + marker = pathlib.Path(out) / "ran" + start = time.monotonic() + r = run_guard(kfd, "--hold", "--max-wait", "2", "--", "touch", str(marker)) + self.assertEqual(r.returncode, 75, r.stderr) + self.assertLess(time.monotonic() - start, 7) + self.assertIn("waiting for guard lock", r.stderr) + self.assertIn("gave up waiting for guard lock", r.stderr) + self.assertFalse(marker.exists()) + self.assertIsNone(first.poll(), "the first hold was disturbed") + + def test_status_reports_held_with_the_holder_then_free(self): + with tempfile.TemporaryDirectory() as kfd: + r = run_guard(kfd, "--status") + self.assertEqual(r.returncode, 0, r.stderr) + self.assertIn("free", r.stdout) + holder = self.start_hold(kfd, "sleep", "8") + for _ in range(100): + if pathlib.Path(f"{_TEST_LOCK}.holder").read_text(): + break + time.sleep(0.05) + r = run_guard(kfd, "--status") + self.assertEqual(r.returncode, 1, r.stdout + r.stderr) + self.assertIn("held", r.stdout) + self.assertIn(f"pid={holder.pid}", r.stdout) + self.assertIn("mode=hold", r.stdout) + self.assertIn("sleep", r.stdout) + self.assertNotIn("stale", r.stdout) + holder.terminate() + holder.communicate(timeout=30) + self.assertEqual(holder.returncode, 143) + r = run_guard(kfd, "--status") + self.assertEqual(r.returncode, 0, r.stdout + r.stderr) + self.assertIn("free", r.stdout) + self.assertFalse(pathlib.Path(f"{_TEST_LOCK}.holder").exists()) + + def test_a_plain_guard_records_and_clears_its_holder_file(self): + with tempfile.TemporaryDirectory() as kfd: + cmd = f"cat {_TEST_LOCK}.holder; exit 2" + r = run_guard(kfd, "--idle-secs", "1", "--", "bash", "-c", cmd) + self.assertEqual(r.returncode, 2, r.stderr) + self.assertIn("mode=guard", r.stdout) + self.assertFalse(pathlib.Path(f"{_TEST_LOCK}.holder").exists()) + + def test_status_calls_a_holder_file_whose_pid_is_gone_stale(self): + gone = subprocess.Popen(["true"]) + gone.wait() + self.hold_lock() + with tempfile.TemporaryDirectory() as kfd: + pathlib.Path(f"{_TEST_LOCK}.holder").write_text( + f"pid={gone.pid}\nstart=then\nmode=hold\ncmd=old\n") + r = run_guard(kfd, "--status") + self.assertEqual(r.returncode, 1, r.stdout) + self.assertIn("stale", r.stdout) + + def test_status_with_a_lock_nobody_recorded_still_says_held(self): + self.hold_lock() + with tempfile.TemporaryDirectory() as kfd: + r = run_guard(kfd, "--status") + self.assertEqual(r.returncode, 1, r.stdout) + self.assertIn("held", r.stdout) + self.assertIn("no holder file", r.stdout) + + def test_hold_refuses_the_idle_options_and_status_refuses_everything(self): + with tempfile.TemporaryDirectory() as kfd: + for extra in (("--idle-secs", "5"), ("--max-attempts", "2"), ("--log", "/dev/null")): + r = run_guard(kfd, "--hold", *extra, "--", "true") + self.assertEqual(r.returncode, 2, (extra, r.stderr)) + self.assertIn("--hold takes only --max-wait", r.stderr) + self.assertEqual(run_guard(kfd, "--status", "--hold").returncode, 2) + self.assertEqual(run_guard(kfd, "--status", "--max-wait", "1").returncode, 2) + self.assertEqual(run_guard(kfd, "--status", "--", "true").returncode, 2) + self.assertEqual(run_guard(kfd, "--hold").returncode, 2) + + def test_hold_under_an_outer_guard_just_runs_the_command(self): + self.hold_lock() + with tempfile.TemporaryDirectory() as kfd: + inner = f"ROCM_GPU_GUARD_LOCK_HELD=$$ bash {GUARD} --hold -- bash -c 'exit 6'; exit $?" + r = subprocess.run(["bash", "-c", inner], env=guard_env(kfd), + capture_output=True, text=True, timeout=60) + self.assertEqual(r.returncode, 6, r.stderr) + self.assertIn("lock held by outer guard", r.stderr) + self.assertNotIn("waiting for guard lock", r.stderr) + + def test_sigterm_stops_the_held_command_and_releases_the_lock(self): + with tempfile.TemporaryDirectory() as kfd, tempfile.TemporaryDirectory() as out: + marker = pathlib.Path(out) / "survived" + p = self.start_hold(kfd, "bash", "-c", f"sleep 20; touch {marker}") + p.terminate() + p.communicate(timeout=30) + self.assertEqual(p.returncode, 143) + self.assertTrue(lock_is_free(_TEST_LOCK)) + self.assertFalse(marker.exists()) + + def test_hold_passes_stdin_to_the_command(self): + with tempfile.TemporaryDirectory() as kfd: + r = subprocess.run(["bash", str(GUARD), "--hold", "--", "cat"], + env=guard_env(kfd), input="piped\n", + capture_output=True, text=True, timeout=60) + self.assertEqual(r.returncode, 0, r.stderr) + self.assertEqual(r.stdout, "piped\n") + def k(op: str) -> str: """A kernel name spelled the way rocprofv3 writes the overlay's kernels."""