[PATCH] perf intel-tpebs: Stop record process before deleting the last entry

[PATCH] perf intel-tpebs: Stop record process before deleting the last entry

[Wenxin Fu]

evsel__tpebs_close() deletes the retire-latency entry from
tpebs_results before calling tpebs_stop(). Sending the STOP command
temporarily drops tpebs_mtx inside tpebs_send_record_cmd(), and in
that window the sample reader thread may still be draining samples
flushed by perf record after STOP. With the entry already gone,
tpebs_retire_lat__find() fails, process_sample_event() returns
-EINVAL, the session aborts and perf_session__delete() closes the
pipe read end early. perf record then gets EPIPE on its final stdout
writes and is killed by SIGPIPE before printing its summary lines,
making perf test "Intel TPEBS counting mode" flaky (~30% failure
rate with 'perf stat -e cache-misses:R --record-tpebs -a sleep 0.01'
in a loop on a 192-core Xeon Platinum 8475B).

Stop the record process while the last entry is still on the list so
the reader can match and drain the remaining samples, and only delete
the entry afterwards.

Fixes: 8db5cabcf1b6 ("perf stat: Fork and launch 'perf record' when 'perf stat' needs to get retire latency value for a metric.")
Signed-off-by: Wenxin Fu <fuwenxin@linux.alibaba.com>
---
 tools/perf/util/intel-tpebs.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
index b41171b5df77..e7a0118bc5e4 100644
--- a/tools/perf/util/intel-tpebs.c
+++ b/tools/perf/util/intel-tpebs.c
@@ -711,11 +711,15 @@ void evsel__tpebs_close(struct evsel *evsel)
 	mutex_lock(tpebs_mtx_get());
 	t = tpebs_retire_lat__find(evsel);
 	if (t) {
+		/*
+		 * Stop while the entry is still visible so the reader thread can
+		 * drain samples flushed after STOP; otherwise it aborts the
+		 * session and closes the pipe, killing perf record with SIGPIPE.
+		 */
+		if (list_is_last(&t->nd, &tpebs_results))
+			tpebs_stop();
 		list_del_init(&t->nd);
 		tpebs_retire_lat__delete(t);
-
-		if (list_empty(&tpebs_results))
-			tpebs_stop();
 	}
 	mutex_unlock(tpebs_mtx_get());
 }
-- 
2.43.7

Re: [PATCH] perf intel-tpebs: Stop record process before deleting the last entry

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf: tpebs reader thread aborts when closing multiple events
--

commit d4a909ca7f9c0a17dd3c8d1e6eee06c407ecc815
Author: Wenxin Fu <fuwenxin@linux.alibaba.com>
perf intel-tpebs: Stop record process before deleting the last entry

This commit modifies the teardown of TPEBS events to stop the record process
before deleting the list entry. The goal is to allow the background reader
thread to drain the remaining samples without failing to find the deleted
event.

> diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> index b41171b5df77d..e7a0118bc5e42 100644
> --- a/tools/perf/util/intel-tpebs.c
> +++ b/tools/perf/util/intel-tpebs.c
> @@ -711,11 +711,15 @@ void evsel__tpebs_close(struct evsel *evsel)
>  	mutex_lock(tpebs_mtx_get());
>  	t = tpebs_retire_lat__find(evsel);
>  	if (t) {
> +		/*
> +		 * Stop while the entry is still visible so the reader thread can
> +		 * drain samples flushed after STOP; otherwise it aborts the
> +		 * session and closes the pipe, killing perf record with SIGPIPE.
> +		 */
> +		if (list_is_last(&t->nd, &tpebs_results))
> +			tpebs_stop();
>  		list_del_init(&t->nd);
>  		tpebs_retire_lat__delete(t);

[Severity: Medium]
Will this abort the session if there are multiple TPEBS events?

If there are multiple events in tpebs_results, evsel__tpebs_close() is called
sequentially for each event. The list_is_last() macro only evaluates to true
for the tail element of the list.

If a non-tail event is closed, list_is_last() is false, so tpebs_stop() is
skipped and the event is immediately deleted from the list. The reader thread
remains running.

If the running reader thread processes a sample for the deleted event, it will
fail in process_sample_event() when tpebs_retire_lat__find() returns NULL:

tools/perf/util/intel-tpebs.c:process_sample_event() {
    ...
	t = tpebs_retire_lat__find(sample->evsel);
	if (!t) {
		mutex_unlock(tpebs_mtx_get());
		return -EINVAL;
	}
    ...
}

Returning -EINVAL here aborts the entire session.

Since tpebs_stop() already protects against being called multiple times, would
it be safer to call tpebs_stop() unconditionally before deleting any entry?
This would ensure the reader thread is completely stopped before any event is
removed from the list.

> -
> -		if (list_empty(&tpebs_results))
> -			tpebs_stop();
>  	}
>  	mutex_unlock(tpebs_mtx_get());
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179057437287.652774.7332965203936908557@t50b09067.sqa.eu95?part=1