Messages in this thread | | | From | Ian Rogers <> | Date | Tue, 9 Apr 2024 09:04:06 -0700 | Subject | Re: [PATCH v1] perf stat: Remove evlist__add_default_attrs use strings |
| |
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.
Thanks, Ian
> Thanks, > Kan >
| |