On Wed, Feb 05, 2025 at 11:44:57PM -0800, Ian Rogers wrote: > On Wed, Feb 5, 2025 at 9:09 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote: > > > > On Tue, Feb 04, 2025 at 08:48:20PM -0800, Ian Rogers wrote: > > > On Tue, Feb 4, 2025 at 5:58 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote: > > You mean the -z flag which is documented in the man page and also it the > > help message (perf top -h). Anyone can read the doc can know it's > > there. Of course, people would prefer reading zines than man pages. :) > > I link to the patch. My point is that something as minor as making > "perf top" behave as "top" does was too big a (user command line) > regression to land - I strongly suspect nobody would notice. Your > proposal breaks all non-core events on every perf command that takes > PMU events. It is a bigger change. I also suspect not much people is using non-core events without PMU. But I won't argue that since I don't have any data. > > > > So, it would seem to me that > > > changing something as fundamental as how all non-core events behave > > > would be seen as a regression. > > > > Yep, it'd be a regression. > > Agreed, you are arguing for a regression. Right, but I thought it won't affect many. But who knows.. And yes, I don't want to create new troubles. > > > Which suffix do you mean? > > It's off topic. ARM added hex suffixes to PMUs representing physical > memory addresses of memory controllers but then that makes cortex_a72 > look like it has a 3 character suffix. So perf assumes hex digits more > than 4 characters long is a hex suffix, which of course it wouldn't be > for a1000 (which is also somewhat close to being an old Acorn > archimedes machine number ;-) ). ok. > > > Anyway, the person looked up the intel webpage would be eager to learn > > about performance related things. Can we also assume if they also want > > to learn about the perf tool itself? :) > > I'm not sure how turning data_read into > uncore_imc_free_running/data_read/ is in anyway helping people > understand perf? They want an event name that matches the > documentation, manual, web site. It is what the vendors I've spoken to > want as they use the event names across tools (fwiw oprofile doesn't > even have a notion of a PMU). To my knowledge the PMU names are the > wild west, often illogical and never mentioned in any kind of > documentation. I have a hard time explaining how the suffixes work and > I believe there are more conventions in the works where there can be > multiple what we are currently calling suffixes. I mean if something doesn't work, they will look 'perf list' and find the event name it supports. For me, PMU name gives a tiny bit more information about the 'data_read' event. But proper decscription for the event is preferred. > > > If it's not the case, we have this: > > > > $ perf record -e xxx > > event syntax error: 'xxx' > > \___ Bad event name > > > > Unable to find event on a PMU of 'xxx' > > Run 'perf list' for a list of valid events > > > > Usage: perf record [<options>] [<command>] > > or: perf record [<options>] -- <command> [<options>] > > > > -e, --event <event> event selector. use 'perf list' to list available events > > > > So it says twice to run 'perf list' to see the events. Then they can > > run either: > > > > $ perf list | grep xxx > > > > or > > > > $ perf list xxx > > > > to see the actual name of the event available in the perf tool. > > Why was adding a PMU to an event name, working around ARM's PMU bug, > such an unsurmontable problem that the original change was reverted? > Because 1 person didn't want to have to write a PMU prefix and > considered it a monumental regression having to do so. Because it's a legacy event 'cycles' and he didn't expect the wildcard behavior? > > > > > > > Even with this what would be the behavior of core events? You want > > > legacy events to have priority over sysfs/json when there is no PMU. > > > You know, and have stated not caring, RISC-V wants different and that > > > it breaks Apple-M's PMUs for a fairly large range of kernel releases > > > including 1 LTS kernel - the only reason I'm writing patches in this > > > area in the 1st place. Software is soft and you can go fix software > > > anywhere in the stack. Listening to vendors and not breaking everyone > > > is the point-of-view these patches have been coming from. I find it > > > very hard to have a conversation where this is just forgotten about > > > and we're working on hypotheticals which seem to be both unwanted and > > > implausible. > > > > Sorry I don't want to repeat that too. Correct me if I'm wrong: > > You are wrong. Hmm.. ok. > > > 1. RISC-V is working on a solution with the current status and it's not > > absoluted needed to change the current behavior. > > They said to you directly it was what they wanted, that's why I > reposted this change and it is, has always been, in the cover letter. > They've then followed up expressing their desire for this behavior but > having to have a plan b as the original change was reverted and you > are blocking this change landing. So they have the plan B. But still prefer overriding legacy with JSON? > > > 2. Apple-M is fixed already. > > No, James tried to repro the bug on a Juno board, not an Apple M, and > didn't succeed. I don't know what kernel he tried. I was told by Mark > Rutland (at LPC) that the tool fix was absolutely necessary and the > PMU driver wouldn't be fixed, hence the series flipping behavior that > I thought Intel would most likely block and wasn't keen to do in the > 1st place (not least wade through all the test behavior changes and > the bug tail). All of this was premised on a threat of reverting all > of the hybrid support so that Apple M could be made to work again, and > I was trying to do a less worse alternative. > https://lore.kernel.org/r/20231123042922.834425-1-irogers@xxxxxxxxxx Sorry, it's not clear to me what's the problem exactly. Can you give me an example command line? > > > > > > > I don't know why people (yourself, Linus) keep wanting to show me the > > > perf list output. It is arbitrary. I rewrote it and changed the > > > behavior of all uncore PMUs within it (we didn't used to deduplicate > > > based on the PMU suffix). It is nice that people think it reads like > > > some religious text. > > > > I think it's what we want users to know how to use the events. > > I don't understand what you are trying to say. I'm saying the behavior > of perf list in its output is arbitrary. We use the same printing code > for every kind of event. An aesthetic decision to put things on a line > does not imply that it is more valid to use or not use a PMU, it just > happens to be what the tool does. Did I break perf list as if you look > in old perf list you see: > ``` > $ perf list > List of pre-defined events (to be used in -e or -M): > > duration_time [Tool event] > ... > ``` > while now you see: > ``` > $ perf list > List of pre-defined events (to be used in -e or -M): > ... > tool: > duration_time > [Wall clock interval time in nanoseconds. Unit: tool] > ... > ``` > I'm hoping people find it useful to have the unit documented. The most important information I think is the name of the event (duration_time). It'd be appropriate if you could call it 'tool/duration_time/' but I'm not sure if it's acceptable cause tool events are not real PMU events. If so, maybe duration_time or tool/duration_time/ ? > > > > Why is the formatting different in perf list for > > > json specified events? Well it is because json events have > > > descriptions and the events you are showing with a PMU don't have a > > > description. I think because there is no description, an effort was > > > made to keep the output compact and put the PMU and event name > > > together. It wasn't trying to enter some kind of long lasting marriage > > > that the event name should only ever be used with the PMU. > > > > I like the description but I don't like the formatting. I think I > > understand why it looks like that but it could be different. Anyway, > > I don't think showing PMU name is related to having descriptions. > > No, it has more to do with how I was feeling about filling in two > string fields called name and alias when rewriting the perf list code. > I added aliases containing the PMU name just to add a little bit more > detail when there seemed to be little documentation with certain > events. I never intended placing the PMU names into any events to be a > commitment that all non-core PMU events would need a PMU prefix and to > break all such people using those events. I think people should use a PMU prefix before wildcard is enabled. > > > > What happens if an event is both in sysfs and json? Well the sysfs event > > > will get the description from the json and then I believe it won't > > > behave as you show. Did the event get broken, as perf list no longer > > > shows it with a PMU, by having a json description written? I think not > > > and I think having descriptions with events is a good thing. > > > > That's bad. Probably we should fix it takes only one of the sources and > > change the JSON event not to clash with sysfs. > > No, you are talking about breaking everything already, let's not break > it yet further - not least as we lack a reasonable way to test it. I > think if you are serious about having such breaking changes then it is > best you add a new command line option, like with libpfm events. I don't want to break things. What's the intended behavior in that case? Thanks, Namhyung
