linux-kernel - Re: [PATCH v1] perf stat: Remove evlist__add_default

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=fW4-uYT=v8U7atuNKUnWx+=2Z+vAyOo_Jy011fuTshqUg@mail.gmail.com>
Date: Tue, 9 Apr 2024 13:38:43 -0700
From: Ian Rogers <irogers@...gle.com>
To: "Liang, Kan" <kan.liang@...ux.intel.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, James Clark <james.clark@....com>, 
	Yang Jihong <yangjihong1@...wei.com>, Kaige Ye <ye@...ge.org>, 
	Yicong Yang <yangyicong@...ilicon.com>, K Prateek Nayak <kprateek.nayak@....com>, 
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] perf stat: Remove evlist__add_default_attrs use strings

On Tue, Apr 9, 2024 at 11:50 AM Liang, Kan <kan.liang@...ux.intel.com> wrote:
>
>
>
> On 2024-04-09 12:04 p.m., Ian Rogers wrote:
> > On Tue, Apr 9, 2024 at 9:00 AM Liang, Kan <kan.liang@...ux.intel.com> wrote:
> >>
> >>
> >>
> >> On 2024-04-09 11:20 a.m., Ian Rogers wrote:
> >>>>> +             ret = parse_events(evlist,
> >>>>> +                             "context-switches,"
> >>>>> +                             "cpu-migrations,"
> >>>>> +                             "page-faults,"
> >>>>> +                             "instructions,"
> >>>>> +                             "cycles,"
> >>>> "cycles",
> >>>> "instructions",
> >>>>
> >>>> It's better to keep the original order.
> >>> So the original order was:
> >>> "cycles,"
> >>> "stalled-cycles-frontend,"
> >>> "stalled-cycles-backend,"
> >>> "instructions"
> >>>
> >>
> >> Right. The stalled-* events are added between default_attrs0 and
> >> default_attrs1.
> >>
> >>
> >>> but many/most/all core PMUs don't provide the stalled-* events. At the
> >>> programmer level instructions is the most fundamental thing, so having
> >>> it last felt wrong hence moving it to be the first after the software
> >>> events. My thought was, if we're going to reorder things then let's
> >>> not do a half measure like:
> >>> "cycles,"
> >>> "instructions,"
> >>> "stalled-cycles-frontend,"
> >>> "stalled-cycles-backend"
> >>>
> >>> let's just put things into their best order. It is obviously easy to
> >>> change but having this way wasn't an accident. There's obviously
> >>> subjectivity about whether cycles is more fundamental than
> >>> instructions, my thought is that you get taught instructions first and
> >>> that these take some number of cycles to execute, hence thinking
> >>> instructions should have some priority in the output over cycles -
> >>> some people may not even know what cycles means, it is hard enough
> >>> when you do given the variety of different clocks 🙂
> >>>
> >>
> >> My concern is that there may be someone who still relies on the std
> >> output of perf stat default. So the output format/order matters for
> >> them. Their scripts probably be broken if the order is changed.
> >
> > I think making everyone suffer for the case of a tool that may behave
> > in this way doesn't make sense. The tool should transition to not care
> > or to say the json output, or at least contribute a test. There is
> > precedent for this attitude, the default metrics for topdown removed
> > the event names in perf stat default output - no one screamed, and I
> > expect that to be the case here.
> >
>
> They did, but that happened after the change was merged. And there was
> no test case for the output at that time.
>
> I agree that if the order is important, there should be a test for it.
> I've emailed the tool owners I know and see if the change impacts them.
> But they are all out of office this week and should be back next week.
> I will let you know regarding their feedback. If the order is important,
> I will update the stat+std_output.sh.

Thanks Kan, just knowing if we should or shouldn't care is progress.

Ian

> Thanks,
> Kan