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

[sashiko-bot@kernel.org]

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