Re: [PATCH v2 1/4] perf tool_pmu: Add memory events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] perf stat: wrong rusage fallback on exited target in
  read_single_counter()
- [Medium] perf tool_pmu: -ENOENT/-EINVAL on exiting thread in
  open_memory_pid_dir()
--

commit 8ab23533dd054a2605a53d9e5d6408d9b222edde
Author: Ian Rogers <irogers@google.com>

perf tool_pmu: Add memory events

This commit adds tool PMU events exposing process and system-wide memory
usage metrics from /proc/<pid>/smaps_rollup and /proc/<pid>/statm, with a
wait4() rusage fallback for memory_rss when a forked workload exits.

> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 37752d6bfef45..d2c8c0ccea495 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -139,6 +139,7 @@ static const char		*output_name;
>  static int			output_fd;
>  static char			*metrics;
>  static struct rusage_stats	ru_stats;
> +static bool			ru_stats_valid;
>  
>  struct perf_stat {
>  	bool			 record;
> @@ -283,12 +284,13 @@ static int read_single_counter(struct evsel *counter, int cpu_map_idx, int threa
>  	int err = evsel__read_counter(counter, cpu_map_idx, thread);
>  
>  	/*
> -	 * Reading user and system time will fail when the process
> -	 * terminates. Use the wait4 values in that case.
> +	 * Reading user, system time and memory_rss will fail when a forked
> +	 * workload terminates. Use the wait4 values in that case.
>  	 */
> -	if (err && cpu_map_idx == 0 &&
> +	if (err && cpu_map_idx == 0 && thread == 0 && ru_stats_valid &&
>  	    (evsel__tool_event(counter) == TOOL_PMU__EVENT_USER_TIME ||
> -	     evsel__tool_event(counter) == TOOL_PMU__EVENT_SYSTEM_TIME)) {
> +	     evsel__tool_event(counter) == TOOL_PMU__EVENT_SYSTEM_TIME ||
> +	     evsel__tool_event(counter) == TOOL_PMU__EVENT_MEMORY_RSS)) {
>  		struct perf_counts_values *count =
>  			perf_counts(counter->counts, cpu_map_idx, thread);
>  		struct perf_counts_values *old_count = NULL;
> @@ -299,10 +301,15 @@ static int read_single_counter(struct evsel *counter, int cpu_map_idx, int threa
>  
>  		if (evsel__tool_event(counter) == TOOL_PMU__EVENT_USER_TIME)
>  			val = ru_stats.ru_utime_usec_stat.mean;
> -		else
> +		else if (evsel__tool_event(counter) == TOOL_PMU__EVENT_SYSTEM_TIME)
>  			val = ru_stats.ru_stime_usec_stat.mean;
> +		else
> +			val = (u64)stat_config.ru_data.ru_maxrss * 1024ULL;
>  
> -		count->val = val;
> +		if (evsel__tool_event(counter) == TOOL_PMU__EVENT_MEMORY_RSS && old_count)
> +			count->val = old_count->val + val;
> +		else
> +			count->val = val;

[Severity: Medium]
Can read_single_counter() substitute the forked command's wait4() rusage
for an exited -p or -t target?

When profiling an explicit target while a child command bounds the
measurement (for example, perf stat -e memory_rss -p <pid> -- sleep 0.1),
reap_workload() -> update_rusage_stats() sets ru_stats_valid = true from
the child command without checking target__none(&target).  If the
monitored target exits before read_counters() runs, evsel__read_counter()
fails on thread == 0 and read_single_counter() reports the child command's
ru_maxrss (or user/system time) as the target's counter value instead of
not counted.  On multi-threaded -p targets, sibling threads have dir_fd ==
-1 after Tgid deduplication and return 0 from evsel__tool_pmu_read(), so
the fallback value succeeds for the whole target.

> diff --git a/tools/perf/util/tool_pmu.c b/tools/perf/util/tool_pmu.c
> index d2f7591747d94..0ae08cd910bc4 100644
> --- a/tools/perf/util/tool_pmu.c
> +++ b/tools/perf/util/tool_pmu.c
[ ... ]
> @@ -203,6 +236,205 @@ static int read_pid_stat_field(int fd, int field, __u64 *val)
[ ... ]
> +static int read_pid_tgid(int dir_fd, int *tgid)
> +{
> +	char buf[1024];
> +	struct io io;
> +	u64 val;
> +	int fd, ret;
> +
> +	fd = openat(dir_fd, "status", O_RDONLY);
> +	if (fd < 0)
> +		return -errno;
> +
> +	io__init(&io, fd, buf, sizeof(buf));
> +	ret = read_proc_status_u64(&io, "Tgid:", &val);
> +	close(fd);
> +	if (!ret)
> +		*tgid = (int)val;
> +	return ret;
> +}
[ ... ]
> @@ -258,19 +490,138 @@ static int tool_pmu__read_stat(struct evsel *evsel, int cpu_map_idx, int thread,
>  	return err;
>  }
>  
> +static int open_memory_pid_dir(enum tool_pmu_event ev, int pid,
> +			       int *seen_tgids, int *nr_seen_tgids, int *out_fd)
> +{
> +	const char *file = "smaps_rollup";
> +	int dir_fd, test_fd, tgid = -1;
> +	char buf[PATH_MAX];
> +
> +	*out_fd = -1;
> +	snprintf(buf, sizeof(buf), "%s/%d", procfs__mountpoint(), pid);
> +	dir_fd = open(buf, O_RDONLY | O_DIRECTORY);
> +	if (dir_fd < 0)
> +		return -errno;
> +
> +	if (seen_tgids) {
> +		int err = read_pid_tgid(dir_fd, &tgid);
> +
> +		if (err) {
> +			close(dir_fd);
> +			return err;
> +		}
> +		for (int i = 0; i < *nr_seen_tgids; i++) {
> +			if (seen_tgids[i] == tgid) {
> +				close(dir_fd);
> +				return 0;
> +			}
> +		}
> +	}
> +
> +	if (ev == TOOL_PMU__EVENT_MEMORY_SIZE ||
> +	    ev == TOOL_PMU__EVENT_MEMORY_DATA ||
> +	    ev == TOOL_PMU__EVENT_MEMORY_TEXT)
> +		file = "statm";
> +
> +	test_fd = openat(dir_fd, file, O_RDONLY);
> +	if (test_fd < 0) {
> +		int err = -errno;
> +
> +		close(dir_fd);
> +		return err;
> +	}

[Severity: Medium]
Does open_memory_pid_dir() returning -ENOENT or -EINVAL when a thread exits
during counter setup cause stat_handle_error() to skip the memory event as
unsupported?

When a thread in the thread map terminates between enumeration and
open_memory_pid_dir(), open() or openat() fails with ENOENT, or read() on
status fails with ESRCH and makes read_proc_status_u64() return -EINVAL.
In builtin-stat.c, stat_handle_error() treats both ENOENT and EINVAL as
COUNTER_SKIP ("event is not supported by the kernel") before reaching the
vanished-thread retry path:

tools/perf/builtin-stat.c:stat_handle_error() {
	if (err == EINVAL || err == ENOSYS || err == ENOENT || err == ENXIO) {
		...
		return COUNTER_SKIP;
	}
	...
	if (target__has_per_thread(&target) &&
	    evlist__core(evsel_list)->threads &&
	    evlist__core(evsel_list)->threads->err_thread != -1) {

Instead of ignoring the exited thread in --per-thread or multi-threaded -p
mode, perf stat skips the memory counter for the entire run.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113233739.3626499-1-irogers@google.com?part=1