Skip to content

Commit aa75a70

Browse files
excelle08meta-codesync[bot]
authored andcommitted
Add start_new_session=True to mpstat Popen to fix perf hook teardown hang (facebookresearch#741)
Summary: Pull Request resolved: facebookresearch#741 The perf hook's `after_job` iterates `AVAIL_MONITORS` in dict order — and `mpstat` is first. `Monitor.terminate()` (D107426991) now does `os.killpg(os.getpgid(self.proc.pid), signal.SIGINT)` to clean up the bash wrapper + perf-stat child cleanly. But when the Popen does NOT pass `start_new_session=True`, the child inherits the PARENT's process group (= benchpress's own group). The killpg then sends SIGINT to **benchpress itself**, raising KeyboardInterrupt inside `terminate()`, aborting cleanup before any subsequent monitor runs, and before `write_csv()` ever fires. Symptom: Run A completes the benchmark (metrics.json + breakdown.csv + perf.data written) but the perf hook produces no CSVs (perf-stat.csv, amd-zen4-perf-collector.csv, etc.). benchpress eventually hits the outer `timeout 3600` and exits with `KeyboardInterrupt` traceback ending at `perf_monitors/__init__.py:115` (the `self.proc.wait(timeout=15)` line). Other monitors that use `subprocess.Popen` (perfstat, topdown's main collectors at lines 187/277/346/505) already have `start_new_session=True` from D107426991. Thread-based monitors (cpufreq_*, memstat, netstat, power, vmstat) override `terminate()` themselves and bypass the killpg path. Only mpstat was unfixed. Reviewed By: YifanYuan3 Differential Revision: D107602859
1 parent 12beee8 commit aa75a70

1 file changed

Lines changed: 11 additions & 1 deletion

File tree

  • benchpress/plugins/hooks/perf_monitors

benchpress/plugins/hooks/perf_monitors/mpstat.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,17 @@ def __init__(self, interval, job_uuid):
1818

1919
def run(self):
2020
args = ["mpstat", "-u", f"{self.interval}"]
21-
self.proc = subprocess.Popen(args, stdout=subprocess.PIPE, encoding="utf-8")
21+
self.proc = subprocess.Popen(
22+
args,
23+
stdout=subprocess.PIPE,
24+
encoding="utf-8",
25+
# CRITICAL: Run in its own process group so Monitor.terminate()
26+
# can killpg() the bash wrapper + mpstat cleanly without
27+
# signalling benchpress itself (which would raise
28+
# KeyboardInterrupt and abort cleanup of all subsequent
29+
# monitors).
30+
start_new_session=True,
31+
)
2232
super(MPStat, self).run()
2333

2434
def process_output(self, line):

0 commit comments

Comments
 (0)