Fix perf hook teardown hang via process-group signaling and bounded waits (#717)

Summary:
Pull Request resolved: #717

The `perf` hook (used as `-k perf` or as part of `-k fb_chef_off_turbo_on fb_stop_dynologd perf`) was hanging indefinitely on benchmark completion. Symptom: after the benchmark JSON metrics had been written, the parent `./benchpress run` Python stayed alive forever waiting on `subprocess.Popen.wait()` for the perf collector bash scripts. On BGM the hung children were `collect_amd_perf_counters.sh` + `collect_amd_zen4_perf_counters.sh`; on Grace the analog was `collect_nvda_neoversev2_perf_counters.sh`. This blocked every overnight automation run that used the perf hook, and required manually `kill -9`-ing the collector PIDs.

Root cause:
`Monitor.terminate()` did `os.kill(self.proc.pid, signal.SIGINT)` followed by an *unbounded* `self.proc.wait()`. But `self.proc` is the bash interpreter PID running the collector script; bash does NOT synchronously forward signals to its `wait`-blocked foreground child (`perf stat`). Even when the collector script's SIGINT trap eventually fires, the parent Python is also blocked on the output-catcher thread's `readline()` until the pipe closes, which only happens after bash exits. On AMD hosts the two simultaneous collectors (`perfutil` + `perfutil_zen4`) compete for the PMU and one of them can hang at startup — its `wait()` therefore never returns. No timeout anywhere → indefinite hang.

Fix (centralized in `Monitor.terminate()`):
1. **Process-group signaling.** `os.killpg(os.getpgid(self.proc.pid), SIGINT)` so both the bash wrapper and its `perf stat` child receive SIGINT in the same atomic delivery. Requires the Popen to have been started with `start_new_session=True` so a new pgid was created.
2. **Bounded waits.** `self.proc.wait(timeout=15)` after SIGINT; if it times out, escalate to `os.killpg(..., SIGKILL)` with another `wait(timeout=5)`. Catcher threads also get `join(timeout=5)`. Worst-case teardown is now ~25s with a logger warning, not indefinite.

Coordinated fix in every long-running `subprocess.Popen` that becomes `self.proc`:
- `perf_monitors/__init__.py` `Monitor.terminate()` — process-group SIGINT + escalating SIGKILL + bounded waits + thread joins with timeouts
- `perf_monitors/perfstat.py:96` (`PerfStat.run`) — add `start_new_session=True`
- `perf_monitors/topdown.py:187` (`IntelPerfSpect.run`) — same
- `perf_monitors/topdown.py:268` (`IntelPerfSpect3.run`) — same
- `perf_monitors/topdown.py:332` (`BasePerfUtil.run` — AMD collectors) — same
- `perf_monitors/topdown.py:505` (`ARMPerfUtil.run` — Grace) — same

Short-lived post-processing Popens (those followed immediately by `.wait()` with no concurrent reader) were left unchanged — they don't suffer from the hang because they're synchronous.

Reviewed By: YifanYuan3

Differential Revision: D107426991

Author: excelle08

benchpress/plugins/hooks/perf_monitors/__init__.py

@@ -98,17 +98,45 @@ def run(self):
 
     def terminate(self):
         """
-        Kill the monitoring process using SIGINT signal and join the stdout
-        and stderr catcher threads.
+        Kill the monitoring process and join its stdout/stderr catcher threads.
+
+        The monitor's `proc` is typically a bash wrapper script (e.g.
+        `perfutils/collect_amd_perf_counters.sh`) that itself spawns `perf
+        stat`. Signaling only the bash PID is unreliable because bash does
+        not synchronously forward signals to its `wait`-blocked foreground
+        child — leaving `perf stat` running and `proc.wait()` hanging
+        forever. We send SIGINT to the entire process group (which requires
+        the Popen to have used `start_new_session=True`) and bound the wait
+        with a timeout, escalating to SIGKILL if SIGINT isn't honored.
         """
         exitcode = -1
         if hasattr(self, "proc") and isinstance(self.proc, subprocess.Popen):
-            os.kill(self.proc.pid, signal.SIGINT)
-            exitcode = self.proc.wait()
+            try:
+                os.killpg(os.getpgid(self.proc.pid), signal.SIGINT)
+            except (ProcessLookupError, PermissionError):
+                pass
+            try:
+                exitcode = self.proc.wait(timeout=15)
+            except subprocess.TimeoutExpired:
+                logger.warning(
+                    f"{getattr(self, 'name', 'Monitor')}: SIGINT did not "
+                    "terminate within 15s, escalating to SIGKILL"
+                )
+                try:
+                    os.killpg(os.getpgid(self.proc.pid), signal.SIGKILL)
+                except (ProcessLookupError, PermissionError):
+                    pass
+                try:
+                    exitcode = self.proc.wait(timeout=5)
+                except subprocess.TimeoutExpired:
+                    logger.error(
+                        f"{getattr(self, 'name', 'Monitor')}: SIGKILL did "
+                        "not reap process within 5s; leaving as orphan"
+                    )
         if hasattr(self, "oc") and isinstance(self.oc, threading.Thread):
-            self.oc.join()
+            self.oc.join(timeout=5)
         if hasattr(self, "ec") and isinstance(self.ec, threading.Thread):
-            self.ec.join()
+            self.ec.join(timeout=5)
         return exitcode
 
     def get_result(self):

benchpress/plugins/hooks/perf_monitors/perfstat.py

@@ -93,5 +93,11 @@ def run(self):
             "--log-fd",
             "1",
         ]
-        self.proc = subprocess.Popen(args, stdout=subprocess.PIPE, encoding="utf-8")
+        self.proc = subprocess.Popen(
+            args,
+            stdout=subprocess.PIPE,
+            encoding="utf-8",
+            # Process-group isolation for clean teardown.
+            start_new_session=True,
+        )
         super(PerfStat, self).run()

benchpress/plugins/hooks/perf_monitors/topdown.py

@@ -185,7 +185,16 @@ def run(self):
             self.collect_output_path,
         ]
         self.proc = subprocess.Popen(
-            args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8"
+            args,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            encoding="utf-8",
+            # Run in its own process group so Monitor.terminate() can killpg()
+            # both the bash wrapper and its perf-stat child cleanly. Without
+            # this, SIGINT to the bash PID alone leaves perf-stat orphaned
+            # and parent .wait() hangs (root cause of the t29/t32 teardown
+            # hang on fb_chef_off_turbo_on + perf hook combo).
+            start_new_session=True,
         )
         super(IntelPerfSpect, self).run()
 
@@ -266,7 +275,12 @@ def run(self):
             self.collect_output_path,
         ]
         self.proc = subprocess.Popen(
-            args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8"
+            args,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            encoding="utf-8",
+            # Process-group isolation for clean teardown (see IntelPerfSpect).
+            start_new_session=True,
         )
         super(IntelPerfSpect3, self).run()
 
@@ -329,7 +343,14 @@ def run(self):
         cmd = [perf_collect_script]
         if self.interval is not None:
             cmd.append(str(self.interval))
-        self.proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, encoding="utf-8")
+        self.proc = subprocess.Popen(
+            cmd,
+            stdout=subprocess.PIPE,
+            encoding="utf-8",
+            # Process-group isolation for clean teardown — Monitor.terminate
+            # killpg()'s this so perf stat + bash wrapper both die together.
+            start_new_session=True,
+        )
         super(BasePerfUtil, self).run()
 
     def gen_csv(self):
@@ -494,6 +515,8 @@ def run(self):
             stdout=subprocess.PIPE,
             stderr=subprocess.PIPE,
             encoding="utf-8",
+            # Process-group isolation for clean teardown.
+            start_new_session=True,
         )
         super(ARMPerfUtil, self).run()