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,