linux-kernel - Re: [PATCH] perf record: Make logs more readable for event open failures

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAP-5=fUwDRxEyEL5A44wAr2RwOhRN6x9ePntpTzWGThyHqFQiw@mail.gmail.com>
Date: Thu, 5 Feb 2026 09:07:35 -0800
From: Ian Rogers <irogers@...gle.com>
To: Leo Yan <leo.yan@....com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, James Clark <james.clark@...aro.org>, 
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf record: Make logs more readable for event open failures

On Thu, Feb 5, 2026 at 2:30 AM Leo Yan <leo.yan@....com> wrote:
>
> On Wed, Feb 04, 2026 at 09:29:14AM -0800, Ian Rogers wrote:
>
> [...]
>
> > > This commit restores evsel__open_strerror() to generate a readable error
> > > message and print it out:
>
> > Lgtm and sorry for making things worse - I believe it was motivated by
> > trying to avoid spammy warnings.
> >
> > Reviewed-by: Ian Rogers <irogers@...gle.com>
>
> Thanks for review, Ian.
>
> Just wander if we can go a bit further.  My understanding is that now we only
> handle the special case of duplicate "cycles" naming on Arm/Arm64, it is
> not necessarily to tolerate other failure cases.
>
> So could we report errors and directly bail out for other PMU event failure?
> This somehow reverts to old neat log rather the duplicated error logs for
> each events.  Something like:
>
> ---8<---
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 2584d0d8bc82..ca7d6805840a 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1404,10 +1404,21 @@ static int record__open(struct record *rec)
>                         }
>  #endif
>                         if (report_error || verbose > 0) {
> +                               evsel__open_strerror(pos, &opts->target, errno, msg, sizeof(msg));
>                                 ui__error("Failure to open event '%s' on PMU '%s' which will be "
>                                           "removed.\n%s\n",
>                                           evsel__name(pos), evsel__pmu_name(pos), msg);
> +
> +                               /*
> +                                * Only tolerate Arm cycle failures and bail out
> +                                * on any other event failures.
> +                                */
> +                               if (report_error) {
> +                                       rc = -errno;
> +                                       goto out;
> +                               }
>                         }
>
>
> If this is okay for you, I can send a updated patch.

My main concern is to not have a code path for ARM that we can't test
on x86. Currently opening an uncore event with a core event on x86
exercises same the cycles event problem as on ARM. The logic I worry
about bit rotting is the evlist clean up, etc:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-record.c?h=perf-tools-next#n1422
If your concern is log spam then we could by default make report_error
true only when no other errors have been reported, but then we may go
on to do the record and be missing removed events that weren't warned
about unless you are in verbose mode.
I think in your patch you'd need to save the errno value to avoid it
being clobbered.

Thanks,
Ian

> Thanks,
> Leo