Skip to content

[Klaud] [AMD][Power] fix: wait for AMD telemetry to cover the benchmark window end before stopping the monitor / 修复 AMD 遥测在基准窗口结束前被截断导致功耗校验失败的问题 #2761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
edwingao28 wants to merge 3 commits into from
Closed

[Klaud] [AMD][Power] fix: wait for AMD telemetry to cover the benchmark window end before stopping the monitor / 修复 AMD 遥测在基准窗口结束前被截断导致功耗校验失败的问题 #2761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
68cc018
fix(power): poll AMD telemetry until it covers the stop request befor…
Aug 27, 2026
41ea427
fix(power): count degenerate boundary rows instead of bracketing or p…
Aug 27, 2026
3c08814
fix(power): harden AMD stop-coverage poll against bad timeout and ms …
Aug 27, 2026
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
142 changes: 128 additions & 14 deletions benchmarks/benchmark_lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ unset _benchmark_caller
GPU_MONITOR_PID=""
GPU_MONITOR_VENDOR=""
GPU_MONITOR_INTERVAL=1
# Bounded wait for AMD telemetry to cover a stop request; 0 skips the wait.
AMD_MONITOR_STOP_TIMEOUT_S="${AMD_MONITOR_STOP_TIMEOUT_S:-30}"
GPU_METRICS_CSV="${GPU_METRICS_CSV:-gpu_metrics.csv}"
NVIDIA_GPU_MONITOR_QUERY="timestamp,index,power.draw,temperature.gpu,clocks.current.sm,clocks.current.memory,utilization.gpu,utilization.memory"
export GPU_METRICS_CSV
Expand Down Expand Up @@ -166,17 +168,18 @@ start_gpu_monitor() {
# Stop the background GPU monitor and report file size.
stop_gpu_monitor() {
if [[ -n "$GPU_MONITOR_PID" ]] && kill -0 "$GPU_MONITOR_PID" 2>/dev/null; then
# benchmark_end_time_unix is recorded shortly before the benchmark
# process exits, so the stream must cover one more sample past it for
# deterministic boundary interpolation. NVIDIA appends a one-shot
# post-exit sample below; amd-smi one-shot CSV has no timestamp column,
# so the AMD path instead lets the watch stream emit final ticks before
# the kill. Two extra intervals: amd-smi stamps integer seconds, so a
# tick in the same second as the window end still fails bracketing —
# the stream needs a tick at the NEXT whole second (measured on MI355X:
# end=...153.325 vs last sample ...153.0).
# The aggregator requires, for every GPU, a usable sample stamped at or
# after the (fractional) benchmark window end, which is always <= the
# wall clock when this stop runs. NVIDIA appends a one-shot post-exit
# sample below; amd-smi one-shot CSV has no timestamp column, so the
# AMD path polls the output file until every GPU's watch stream shows
# a usable tick at the next whole second — amd-smi stamps integer
# seconds, so that tick strictly covers any fractional window end
# (measured on MI355X: end=...609.157 vs last sample ...605). Observing
# the file rather than sleeping also defeats pipe-buffer loss when the
# awk consumer is killed: covered rows are already on disk.
if [[ "$GPU_MONITOR_VENDOR" == "amd" ]]; then
sleep $(( ${GPU_MONITOR_INTERVAL:-1} + 2 ))
_wait_for_amd_stop_coverage
fi
kill "$GPU_MONITOR_PID" 2>/dev/null
wait "$GPU_MONITOR_PID" 2>/dev/null || true
Expand Down Expand Up @@ -222,6 +225,109 @@ _repair_truncated_gpu_metrics_tail() {
return 0
}

# Print the newest telemetry tick (whole epoch seconds) that EVERY observed
# GPU has covered with a usable sample (numeric epoch timestamp, numeric
# power > 0), or nothing when the stream holds no usable epoch-stamped row
# (e.g. an amd-smi build emitting ISO timestamps). Column detection mirrors
# _POWER_COL_RE/_POWER_EXCLUDE_RE/_GPU_INDEX_COL_RE in utils/aggregate_power.py.
# POSIX awk only: the ROCm container images ship mawk/busybox awk.
_amd_monitor_min_covered_tick() {
[[ -f "$GPU_METRICS_CSV" ]] || return 0
awk -F, '
NR == 1 {
for (i = 1; i <= NF; i++) {
name = tolower($i)
gsub(/^ +| +$/, "", name)
sub(/\r$/, "", name)
if (!power_col && name ~ /power/ && name !~ /limit|cap|max|min/)
power_col = i
if (!gpu_col && name ~ /^(index|gpu|gpu_id|gpu_index|card|device)$/)
gpu_col = i
}
next
}
!power_col || !gpu_col { next }
{
# amd-smi quotes list-valued cells that embed commas; neutralize
# them so the power cell keeps its header-relative position.
line = $0
sub(/\r$/, "", line)
if (line ~ /"/) {
n = split(line, seg, /"/)
line = ""
for (i = 1; i <= n; i++) {
if (i % 2 == 0) gsub(/,/, ";", seg[i])
line = line seg[i]
}
}
count = split(line, cell, /,/)
if (count < power_col || count < gpu_col) next
if (cell[1] !~ /^[0-9]+(\.[0-9]+)?$/) next
if (cell[power_col] !~ /^[0-9]+(\.[0-9]+)?$/) next
if (cell[power_col] + 0 <= 0) next
if (cell[gpu_col] == "") next
ts = cell[1] + 0
# Mirror _parse_timestamp in utils/aggregate_power.py: normalize
# millisecond epochs so a ms-stamping amd-smi build cannot
# trivially satisfy any second-scale stop target.
if (ts > 1e12) ts /= 1000
gpu = cell[gpu_col]
if (!(gpu in newest) || ts > newest[gpu])
newest[gpu] = ts
}
END {
have = 0
for (gpu in newest)
if (!have || newest[gpu] < min) { min = newest[gpu]; have = 1 }
if (have) printf "%d\n", min
}
' "$GPU_METRICS_CSV" 2>/dev/null
return 0
}

# Block until every observed GPU has a usable tick at/after the first whole
# second past stop entry, so any window end preceding the stop request is
# bracketed on file. Bounded by AMD_MONITOR_STOP_TIMEOUT_S; always returns 0 —
# on timeout or early monitor death it warns and lets aggregation attribute
# the missing coverage (fail-safe, never fail-silent).
_wait_for_amd_stop_coverage() {
local target deadline covered timeout_s
# A non-integer timeout (e.g. "30s") would abort the whole stop_gpu_monitor
# call under `set -e` at the arithmetic below, leaking the monitor process
# and skipping tail repair + the energy sidecar; fall back to the default.
timeout_s="${AMD_MONITOR_STOP_TIMEOUT_S:-30}"
if [[ ! "$timeout_s" =~ ^-?[0-9]+$ ]]; then
echo "[GPU Monitor] Warning: ignoring non-integer AMD_MONITOR_STOP_TIMEOUT_S='$timeout_s', using 30" >&2
timeout_s=30
fi
if [[ "$timeout_s" -le 0 ]]; then
return 0
fi
target=$(( $(date +%s) + 1 ))
deadline=$(( target + timeout_s ))
Comment on lines +298 to +307

@claude claude Bot Aug 27, 2026

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.

🟡 _wait_for_amd_stop_coverage's timeout sanitization regex ^-?[0-9]+$ (line 299) accepts leading-zero digit strings like "08"/"09", but bash arithmetic treats a leading-0 numeral as octal; digits 8/9 make it an invalid octal literal. The very next arithmetic uses of $timeout_s -- [[ "$timeout_s" -le 0 ]] (line 303) and deadline=$(( target + timeout_s )) (line 307) -- both perform bash arithmetic evaluation and hit "value too great for base", which is a fatal bash expansion error that aborts the shell outright (not just a non-zero return), independent of set -e.

Extended reasoning...

Set AMD_MONITOR_STOP_TIMEOUT_S=08 (or any leading-zero value containing an 8/9 digit, e.g. "09", "018"). The regex guard added by this PR to catch non-integer timeouts (and specifically tested by test_amd_stop_survives_non_integer_timeout for "30s") lets "08" through as "valid", so no fallback-to-30 warning fires. The subsequent -le 0 test / deadline=$((...)) arithmetic then throws bash's octal "value too great for base" error, which is a fatal, non-interactive-shell-terminating error class -- reproducing (and likely worsening, since it can kill the whole caller script rather than just stop_gpu_monitor) the exact leaked-monitor/skipped-tail-repair/skipped-energy-sidecar failure this PR's sanitization was specifically added to prevent. Fix: strip/reject leading zeros (or force base-10 with 10#$timeout_s) before using $timeout_s in any arithmetic context.

Verification: Severity: nit. The regex at benchmark_lib.sh:299 (^-?[0-9]+$) accepts leading-zero strings like "08"/"09"/"018", so the non-integer fallback (line 300-301) does not fire. Bash arithmetic then treats a leading-0 numeral as octal, and digits 8/9 make it an invalid octal literal. Line 303 [[ "$timeout_s" -le 0 ]] is an if condition (set -e exempt: it prints the error but continues),…

while :; do
covered=$(_amd_monitor_min_covered_tick)
if [[ -z "$covered" ]]; then
# Non-epoch timestamps or an unusable stream: keep the legacy
# fixed tail so older amd-smi builds behave exactly as before.
sleep $(( ${GPU_MONITOR_INTERVAL:-1} + 2 ))
return 0
fi
if [[ "$covered" -ge "$target" ]]; then
return 0
fi
if ! _background_process_is_running "$GPU_MONITOR_PID"; then
echo "[GPU Monitor] Warning: AMD monitor exited before covering the stop request (covered=$covered target=$target)" >&2
return 0
fi
if [[ "$(date +%s)" -ge "$deadline" ]]; then
echo "[GPU Monitor] Warning: AMD telemetry never covered the stop request within ${timeout_s}s (covered=$covered target=$target)" >&2
return 0
fi
sleep 1
done
}

# Write one best-effort amd-smi snapshot; remove the file rather than keep a
# partial one when the invocation fails.
_write_amd_smi_sidecar() {
Expand Down Expand Up @@ -2206,8 +2312,14 @@ run_agentic_replay_and_write_outputs() (
esac

_stop_agentx_power_monitor() {
local mode="${1:-}"
if [ "$agentx_monitor_stopped" = "0" ]; then
agentx_monitor_stopped=1
if [ "$mode" = "abort" ]; then
# A cancelled run's power validity is moot; skip the AMD
# coverage wait so signal teardown stays fast.
AMD_MONITOR_STOP_TIMEOUT_S=0
fi
stop_gpu_monitor
fi
}
Expand Down Expand Up @@ -2252,10 +2364,12 @@ run_agentic_replay_and_write_outputs() (
agentx_monitor_stopped=0
# This function runs in a subshell, so these handlers cannot replace
# launcher-owned traps. The stopped flag keeps explicit and signal/EXIT
# cleanup idempotent.
trap '_stop_agentx_power_monitor' EXIT
trap '_stop_agentx_power_monitor; exit 130' INT
trap '_stop_agentx_power_monitor; exit 143' TERM
# cleanup idempotent. Abort mode only ever fires when the explicit
# post-replay stop did not run (abnormal exit), where the coverage
# wait would only slow teardown down.
trap '_stop_agentx_power_monitor abort' EXIT
trap '_stop_agentx_power_monitor abort; exit 130' INT
trap '_stop_agentx_power_monitor abort; exit 143' TERM
fi

echo "$REPLAY_CMD" > "$result_dir/benchmark_command.txt"
Comment on lines +2370 to 2375

@claude claude Bot Aug 27, 2026

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.

🟣 The explicit post-replay stop (_stop_agentx_power_monitor at line 2385) runs stop_gpu_monitor's now-up-to-30s AMD coverage wait (_wait_for_amd_stop_coverage, AMD_MONITOR_STOP_TIMEOUT_S default 30) while the INT/TERM traps installed at lines 2370-2372 are still active (they're only removed at line 2386, after the call returns). If SIGINT/SIGTERM arrives during that wait, the trap fires _stop_agentx_power_monitor abort; exit N; since agentx_monitor_stopped was already set to 1 at function entry (line 2317, before calling stop_gpu_monitor), the trap's guarded body is skipped but the trailing exit 130/exit 143 still runs unconditionally, killing the subshell immediately.

Extended reasoning...

A user Ctrl-C's (or a scheduler SIGTERMs) the benchmark while the explicit post-replay stop is mid-poll waiting for AMD telemetry coverage. The process exits via the trap's exit 130/143 before stop_gpu_monitor reaches kill/wait on GPU_MONITOR_PID, tail repair, or the end-of-run amd-smi energy sidecar — leaking the orphaned awk/amd-smi consumer process and dropping the energy_end.csv artifact. This same trap-during-explicit-stop race existed pre-diff too, but was bounded to the old fixed ~3s sleep window; this diff widens the exposed window roughly 10x (up to 30s by default) by design, making the previously-negligible race a realistically triggerable teardown gap. A correct fix must make the explicit stop itself not re-enter abort-truncation (e.g. temporarily disable/replace the INT/TERM traps for the duration of the explicit stop, or have stop_gpu_monitor ignore/defer signals during its wait) rather than leaving the pre-existing unconditional exit reachable mid-wait.

Verification: pre-existing. The race is real and reachable: line 2385 calls _stop_agentx_power_monitor (full-wait mode) while the INT/TERM traps at lines 2371-2372 are still armed (removed only at 2386). _stop_agentx_power_monitor sets agentx_monitor_stopped=1 before calling stop_gpu_monitor, which on the AMD path enters _wait_for_amd_stop_coverage (a while :; ... sleep 1 loop bounded by…

Expand Down
Loading