Received-SPF: pass (google.com: domain of linux-kernel+bounces-194519-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240510053705.2462258-1-irogers@google.com> <20240510053705.2462258-4-irogers@google.com>
 <35d6875b-9a28-4953-a187-b6659880ac66@arm.com>
In-Reply-To: <35d6875b-9a28-4953-a187-b6659880ac66@arm.com>
From: Ian Rogers <irogers@google.com>
Date: Wed, 29 May 2024 10:39:32 -0700
Message-ID: <CAP-5=fUbZRkoRN__ZE3YmzhM5HqQABZ7jHymgshZGDzT0LgqOg@mail.gmail.com>
Subject: Re: [PATCH v3 3/5] perf stat: Remove evlist__add_default_attrs use strings
To: James Clark <james.clark@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, 
	Arnaldo Carvalho de Melo <acme@kernel.org>, Namhyung Kim <namhyung@kernel.org>, 
	Mark Rutland <mark.rutland@arm.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, Kan Liang <kan.liang@linux.intel.com>, 
	Ravi Bangoria <ravi.bangoria@amd.com>, Yang Jihong <yangjihong@bytedance.com>, 
	Ze Gao <zegao2021@gmail.com>, Leo Yan <leo.yan@linux.dev>, Song Liu <song@kernel.org>, 
	K Prateek Nayak <kprateek.nayak@amd.com>, Kaige Ye <ye@kaige.org>, 
	Yicong Yang <yangyicong@hisilicon.com>, linux-perf-users@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, May 29, 2024 at 8:39=E2=80=AFAM James Clark <james.clark@arm.com> w=
rote:
>
>
>
> On 10/05/2024 06:37, Ian Rogers wrote:
> > add_default_atttributes would add evsels by having pre-created
> > perf_event_attr, however, this needed fixing for hybrid as the
> > extended PMU type was necessary for each core PMU. The logic for this
> > was in an arch specific x86 function and wasn't present for ARM,
> > meaning that default events weren't being opened on all PMUs on
> > ARM. Change the creation of the default events to use parse_events and
> > strings as that will open the events on all PMUs.
> >
> > Rather than try to detect events on PMUs before parsing, parse the
> > event but skip its output in stat-display.
> >
> > The previous order of hardware events was: cycles,
> > stalled-cycles-frontend, stalled-cycles-backend, instructions. As
> > instructions is a more fundamental concept the order is changed to:
> > instructions, cycles, stalled-cycles-frontend, stalled-cycles-backend.
> >
> > Closes: https://lore.kernel.org/lkml/CAP-5=3DfVABSBZnsmtRn1uF-k-G1GWM-L=
5SgiinhPTfHbQsKXb_g@mail.gmail.com/
>
> Taking a look at this one now. I think some example commands and outputs
> in the commit message would be helpful because there are quite a few
> different things mentioned in the closes link.
>
> But I'm assuming this is just for the command without specifying an event=
:
>
>   $ perf stat
>
> I didn't realise that wasn't working properly and I'd missed that Mark
> spotted it in that link.

Hi James,

Do you want to take on owning fixing this? My concern is that by using
event parsing:

+                               "context-switches,"
+                               "cpu-migrations,"
+                               "page-faults,"
+                               "instructions,"
+                               "cycles,"
+                               "stalled-cycles-frontend,"
+                               "stalled-cycles-backend,"
+                               "branches,"
+                               "branch-misses",

any of the names could conflict with something advertised on an ARM
PMU somewhere. Clearly cycles has already proved to be controversial
and broken for perf record on Neoverse.

Thanks,
Ian