From 7dc9b9baef9b1a6caa8b3b714ef0d6125104b51c Mon Sep 17 00:00:00 2001 From: Yuxin Chen Date: Fri, 10 Jul 2026 08:12:04 +0000 Subject: [PATCH] fix: improve monitor correctness and robustness Four fixes to nvlink_monitor: 1. Async-signal-safe signal handler (monitor/nvlink_monitor.{h,cpp}): g_running changed from volatile bool to volatile sig_atomic_t so writes from the signal handler are well-defined. The handler no longer calls std::cout (not async-signal-safe; can deadlock if the signal interrupts the main thread mid-output) -- it only flips the flag. The "exiting" notice is printed from main() after the monitoring loop observes the flag, covering both continuous and single modes. 2. Timestamp consistency in runContinuousMonitoring (monitor/nvlink_monitor.cpp): the per-iteration timestamp is now recorded AFTER getNvLinkData() returns, matching the pre-loop snapshot's timestamp. Previously the timestamp was taken before the read, so iteration 1 excluded the read duration while iteration 2+ included it, producing inconsistent actualInterval deltas. Now every actualInterval exactly equals the observation window between two read completions. 3. NVML fallback counter unit caveat (monitor/nvlink_monitor.cpp): the traditional nvmlDeviceGetNvLinkUtilizationCounter fallback returns counters whose units depend on nvmlDeviceSetNvLinkUtilizationCounter config and are not guaranteed to be KiB like the Field Values API. Documented this in a code comment and gated the per-link debug print behind verbose mode (it previously spammed stdout unconditionally in continuous mode) with a warning noting the bandwidth estimate may be inaccurate. 4. Counter reset wording and handling (monitor/bandwidth_calc.cpp): a negative delta on the 64-bit KiB throughput counters (~8 EiB wrap range) almost always indicates a driver counter reset, not arithmetic overflow. Renamed the warning from "overflow" to "counter reset", added the delta value to the message, and added a comment explaining that no meaningful rate can be computed across a reset so the sample is clamped to 0. Renamed the test CounterOverflowClampedToZero to CounterResetClampedToZero and added a CounterResetVerboseWarningDoesNotCrash test exercising the verbose stderr branch. Tests: 59 pass (was 58; +1 new). make check-format clean. make monitor builds with -Wall -Wextra and no warnings. Signed-off-by: staryxchen --- monitor/bandwidth_calc.cpp | 19 +++++++++---- monitor/nvlink_monitor.cpp | 54 ++++++++++++++++++++++++++++-------- monitor/nvlink_monitor.h | 6 ++-- test/test_bandwidth_calc.cpp | 16 +++++++++-- 4 files changed, 75 insertions(+), 20 deletions(-) diff --git a/monitor/bandwidth_calc.cpp b/monitor/bandwidth_calc.cpp index eb797f8..654b564 100644 --- a/monitor/bandwidth_calc.cpp +++ b/monitor/bandwidth_calc.cpp @@ -32,20 +32,29 @@ std::vector calculateBandwidth( long long rxDelta = static_cast(link2.rxBytes) - static_cast(link1.rxBytes); - // Handle overflow cases with detailed logging + // A negative delta means the counter went backwards. The NVML + // throughput counters are 64-bit KiB values (~8 EiB wrap range), + // so genuine arithmetic overflow is effectively impossible — a + // negative delta almost always indicates the driver reset the + // counter (e.g. on certain driver events). No meaningful rate can + // be computed across a reset, so we clamp to 0 for this sample + // and warn so the user can filter reset samples out of steady- + // state averages rather than treating them as idle links. if (txDelta < 0) { if (verbose) { - std::cerr << "Warning: TX counter overflow detected on GPU " + std::cerr << "Warning: TX counter reset detected on GPU " << s2.gpuId << " Link " << link2.linkId - << std::endl; + << " (delta=" << txDelta + << "); sample rate set to 0" << std::endl; } txDelta = 0; } if (rxDelta < 0) { if (verbose) { - std::cerr << "Warning: RX counter overflow detected on GPU " + std::cerr << "Warning: RX counter reset detected on GPU " << s2.gpuId << " Link " << link2.linkId - << std::endl; + << " (delta=" << rxDelta + << "); sample rate set to 0" << std::endl; } rxDelta = 0; } diff --git a/monitor/nvlink_monitor.cpp b/monitor/nvlink_monitor.cpp index e1c174d..dc0cbc4 100644 --- a/monitor/nvlink_monitor.cpp +++ b/monitor/nvlink_monitor.cpp @@ -7,14 +7,17 @@ #include "arg_parser.h" #include "bandwidth_calc.h" -// Global flag for signal handling -volatile bool g_running = true; - -// Signal handler implementation +// Global flag for signal handling. sig_atomic_t guarantees that writes from +// the signal handler are well-defined. The handler only flips this flag — it +// must NOT do any I/O (std::cout/cerr are not async-signal-safe and can +// deadlock if the signal interrupts the main thread mid-output). +volatile sig_atomic_t g_running = 1; + +// Signal handler implementation — async-signal-safe: only flips g_running. +// The "exiting" notice is printed by the main loop after it observes the flag. void signal_handler(int signal) { if (signal == SIGINT || signal == SIGTERM) { - std::cout << "\nReceived stop signal, exiting..." << std::endl; - g_running = false; + g_running = 0; } } @@ -157,16 +160,30 @@ std::vector NvLinkMonitor::getNvLinkData() { linkData.rxBytes = fieldValues[1].value.ullVal; } } else { - // Fallback to traditional API + // Fallback to the traditional utilization-counter API. + // CAVEAT: unlike NVML_FI_DEV_NVLINK_THROUGHPUT_DATA_TX/RX + // (which are documented as KiB throughput), the raw + // counters from nvmlDeviceGetNvLinkUtilizationCounter have + // units that depend on the counter configuration set via + // nvmlDeviceSetNvLinkUtilizationCounter, and are not + // guaranteed to be KiB. We feed them through the same + // KiB->GiB conversion in bandwidth_calc as a best-effort + // estimate, so bandwidth numbers from this path may be + // inaccurate. Warn once per link in verbose mode. unsigned long long rxCounter, txCounter; if (nvmlDeviceGetNvLinkUtilizationCounter( gpu.device, link, 0, &rxCounter, &txCounter) == NVML_SUCCESS) { linkData.rxBytes = rxCounter; linkData.txBytes = txCounter; - std::cout << " Link " << link - << " (Traditional): TX=" << linkData.txBytes - << " RX=" << linkData.rxBytes << std::endl; + if (verboseOutput) { + std::cerr + << "Warning: GPU " << gpu.id << " Link " << link + << " using traditional utilization counter " + << "(units may differ from KiB throughput; " + << "bandwidth estimate may be inaccurate)" + << std::endl; + } } else { std::cerr << "Failed to get utilization counters for GPU " @@ -275,8 +292,16 @@ void NvLinkMonitor::runContinuousMonitoring(double interval) { if (!g_running) break; - auto currentTime = std::chrono::high_resolution_clock::now(); + // Record the timestamp AFTER reading the counters so that both + // lastTime and currentTime mark the moment a counter read completed. + // actualInterval then exactly equals the observation window between + // two reads (which includes the previous iteration's calculate/print + // time — NVML counters keep accumulating during that work, so it + // belongs in the denominator). Recording the timestamp before the + // read would make iteration 1 exclude the read duration while + // iteration 2+ include it, producing inconsistent deltas. auto currentSnapshot = getNvLinkData(); + auto currentTime = std::chrono::high_resolution_clock::now(); // Calculate actual time difference with nanosecond precision auto timeDiff = std::chrono::duration_cast( @@ -407,5 +432,12 @@ int main(int argc, char* argv[]) { return 1; } + // Printed from the main thread (not the signal handler) because std::cout + // is not async-signal-safe. The handler only flips g_running; this covers + // both continuous and single monitoring modes uniformly. + if (!g_running) { + std::cout << "\nReceived stop signal, exiting..." << std::endl; + } + return 0; } \ No newline at end of file diff --git a/monitor/nvlink_monitor.h b/monitor/nvlink_monitor.h index 802d32a..9933ece 100644 --- a/monitor/nvlink_monitor.h +++ b/monitor/nvlink_monitor.h @@ -12,8 +12,10 @@ #include #include -// Global flag for signal handling -extern volatile bool g_running; +// Global flag for signal handling. Uses sig_atomic_t (not bool) so that +// writes from the async signal handler are well-defined per the C/C++ +// standard; the handler must stay async-signal-safe (no I/O, no allocations). +extern volatile sig_atomic_t g_running; // Signal handler declaration void signal_handler(int signal); diff --git a/test/test_bandwidth_calc.cpp b/test/test_bandwidth_calc.cpp index 8c7d92a..30a4d73 100644 --- a/test/test_bandwidth_calc.cpp +++ b/test/test_bandwidth_calc.cpp @@ -50,8 +50,10 @@ TEST(CalculateBandwidth, ZeroDelta) { EXPECT_NEAR(r[0].totalTxGiBps, 0.0, 1e-9); } -TEST(CalculateBandwidth, CounterOverflowClampedToZero) { - // s2 < s1 simulates counter overflow / wraparound. +TEST(CalculateBandwidth, CounterResetClampedToZero) { + // s2 < s1 simulates a driver counter reset (the 64-bit KiB throughput + // counters don't realistically wrap, so a negative delta means reset). + // No meaningful rate can be computed across a reset, so it is clamped to 0. auto s1 = makeGpu("0", 1, {makeLink(0, 1000000, 1000000)}); auto s2 = makeGpu("0", 1, {makeLink(0, 100, 100)}); auto r = calculateBandwidth({s1}, {s2}, 1.0, false); @@ -61,6 +63,16 @@ TEST(CalculateBandwidth, CounterOverflowClampedToZero) { EXPECT_NEAR(r[0].totalTxGiBps, 0.0, 1e-9); } +TEST(CalculateBandwidth, CounterResetVerboseWarningDoesNotCrash) { + // Verbose mode emits a warning for a reset but must still produce a valid + // zero-rate result (exercises the verbose stderr branch). + auto s1 = makeGpu("0", 1, {makeLink(0, 1000000, 0)}); + auto s2 = makeGpu("0", 1, {makeLink(0, 100, 0)}); + auto r = calculateBandwidth({s1}, {s2}, 1.0, true); + ASSERT_EQ(r.size(), 1u); + EXPECT_NEAR(r[0].links[0].txGiBps, 0.0, 1e-9); +} + TEST(CalculateBandwidth, MultiLinkAggregation) { auto s1 = makeGpu("0", 2, {makeLink(0, 0, 0), makeLink(1, 0, 0)}); auto s2 = makeGpu("0", 2,