Re: [PATCH v3 2/3] perf pmu: Implement tool-provided NVMe PMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] perf pmu: missing errno and err_thread handling in
  evsel__nvme_pmu_open()
- [Medium] perf evsel: PERF_EVENT_IOC_ENABLE/DISABLE ioctls issued on
  NVMe device fds
--

commit 1cca3c688611b8ef7867f412d383cb3ae7a315e1
Author: Ian Rogers <irogers@google.com>

perf pmu: Implement tool-provided NVMe PMU

This commit adds a tool-provided PMU for NVMe controllers using libnvme,
exposing metrics from standard NVMe log pages under nvme_<ctrl> PMUs.

> diff --git a/tools/perf/util/nvme_pmu.c b/tools/perf/util/nvme_pmu.c
> new file mode 100644
> index 0000000000000..3ee967505dec6
> --- /dev/null
> +++ b/tools/perf/util/nvme_pmu.c

[ ... ]

> +static int nvme_pmu__read_val(int fd, uint64_t config, uint64_t *val)
> +{

[ ... ]

> +	if (ret < 0)
> +		return -errno ?: -EIO;
> +	if (ret > 0)
> +		return -EOPNOTSUPP;

[ ... ]

> +int evsel__nvme_pmu_open(struct evsel *evsel,
> +			 struct perf_thread_map *threads,
> +			 int start_cpu_map_idx, int end_cpu_map_idx)
> +{
> +	struct nvme_pmu *nvm = container_of(evsel->pmu, struct nvme_pmu, pmu);
> +	int idx = start_cpu_map_idx, thread = 0, nthreads, err = 0;
> +	int max_cpus, max_threads;
> +	char path[PATH_MAX];
> +
> +	if (evsel->core.attr.sample_period || !evsel->core.fd)
> +		return -EINVAL;
> +
> +	snprintf(path, sizeof(path), "/dev/%s", nvm->dev_name);

[ ... ]

> +	for (idx = start_cpu_map_idx; idx < end_cpu_map_idx; idx++) {
> +		for (thread = 0; thread < nthreads; thread++) {
> +			uint64_t *initial_val;
> +			int fd = open(path, O_RDONLY | O_CLOEXEC);
> +
> +			FD(evsel, idx, thread) = fd;

[Severity: Medium]
Does storing a /dev/nvmeX file descriptor in evsel->core.fd cause
evsel__enable_cpu(), evsel__enable(), evsel__disable_cpu() and
evsel__disable() to fail with -ENOTTY?

Those functions only special-case evsel__is_tool(evsel) and otherwise call
 into libperf:

tools/perf/util/evsel.c:evsel__enable_cpu() {
	if (evsel__is_tool(evsel))
		err = evsel__tool_pmu_enable_cpu(evsel, cpu_map_idx);
	else
		err = perf_evsel__enable_cpu(&evsel->core, cpu_map_idx);

	if (!err && evsel__is_group_leader(evsel)) {
    ...
}

perf_evsel__enable_cpu() and its enable/disable counterparts issue
PERF_EVENT_IOC_ENABLE and PERF_EVENT_IOC_DISABLE on every fd in
evsel->core.fd, which the NVMe character device rejects with -ENOTTY.
That leaves evsel->disabled stale and aborts the group loop before
userspace members such as duration_time, user_time or system_time are
enabled or disabled in mixed groups.

> +			if (fd < 0) {
> +				err = -errno;
> +				goto out_close;
> +			}
> +			initial_val = xyarray__entry(evsel->nvme.initial_values, idx, thread);
> +			err = nvme_pmu__read_val(fd, evsel->core.attr.config, initial_val);
> +			if (err) {
> +				close(fd);
> +				FD(evsel, idx, thread) = -1;
> +				goto out_close;
> +			}
> +		}
> +	}
> +	return 0;
> +out_close:
> +	if (err)
> +		threads->err_thread = thread;
> +
> +	do {
> +		while (--thread >= 0) {
> +			if (FD(evsel, idx, thread) >= 0)
> +				close(FD(evsel, idx, thread));
> +			FD(evsel, idx, thread) = -1;
> +		}
> +		thread = nthreads;
> +	} while (--idx >= start_cpu_map_idx);
> +	return err;
> +}

[Severity: Medium]
Does evsel__nvme_pmu_open() leave errno unset or clobbered on failure while
also setting threads->err_thread for a controller-wide device?

Callers of evsel__open() and evsel__open_per_cpu_and_thread() such as
__run_perf_stat(), record__open() and __cmd_top() read errno after a
negative return.  When a controller does not support an optional log page
(such as FDP, ZNS or Endurance Group), libnvme returns a positive NVMe
status code without setting errno, and nvme_pmu__read_val() returns
-EOPNOTSUPP with errno still 0 (or clobbered by close()).  The -EINVAL and
!HAVE_LIBNVME_SUPPORT error returns likewise do not set errno.

In addition, setting threads->err_thread = thread causes
stat_handle_error() in perf stat --per-thread mode to treat a controller
open or log error as a dead thread, removing every thread from the thread
map in a retry loop until nthreads reaches 0 and evsel__nvme_pmu_open()
returns 0 with no descriptors opened.

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