Messages in this thread | ![/](/images/icornerl.gif) | | From | Ian Rogers <> | Date | Tue, 9 Apr 2024 13:38:43 -0700 | 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@linux.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@linux.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
| ![\](/images/icornerr.gif) |