fix: improve monitor correctness and robustness

monitor/bandwidth_calc.cpp

@@ -32,20 +32,29 @@ std::vector<GPUMonitorResult> calculateBandwidth(
             long long rxDelta = static_cast<long long>(link2.rxBytes) -
                                 static_cast<long long>(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;
             }

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<GPUMonitorResult> 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<std::chrono::nanoseconds>(
@@ -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;
 }

monitor/nvlink_monitor.h

@@ -12,8 +12,10 @@
 #include <thread>
 #include <vector>
 
-// 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);

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,