Messages in this thread | | | From | Namhyung Kim <> | Date | Tue, 29 Nov 2022 14:45:55 -0800 | Subject | Re: [PATCH 14/15] perf stat: Rename "aggregate-number" to "cpu-count" in JSON |
| |
On Sat, Nov 26, 2022 at 7:14 PM Ian Rogers <irogers@google.com> wrote: > > On Thu, Nov 24, 2022 at 11:51 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > Hi Ian, > > > > On Wed, Nov 23, 2022 at 3:31 PM Ian Rogers <irogers@google.com> wrote: > > > > > > On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote: > > > > > > > > As the JSON output has been broken for a little while, I guess there are > > > > not many users. Let's rename the field to more intuitive one. :) > > > > > > I'm not sure cpu-count is accurate. For example, an uncore counter in > > > a dual socket machine may have a CPU mask of "0, 36", ie one event per > > > socket. The aggregate-number in this case I believe is 2. > > > > You're right. In case of uncore events, it can be confusing. But in some > > sense it could be thought as cpu count as well since it aggregates the > > result from two cpus anyway. :) > > > > Note that the aggregate-number (or cpu-count) is only printed if users > > requested one of aggregation options like --per-socket or --per-core. > > In your example, then it could print 1 for each socket. > > > > But I think uncore events are different from core events, and hopefully > > they have separate instances for different sockets or something already. > > That means it doesn't need to use those aggregation options for them. > > > > Also the CSV output uses "cpus" for the same information. It'd be nice > > we could have consistency. > > So in the original patch from Claire she'd passed the name "number" > through to the json from the stat code. Having an integer called > "number" isn't exactly intention revealing - thank you for your clean > up work! :-) I switched "number" to be "aggregate number" as the > number comes from the "data" aggregated and the code refers to it as > aggregate data. I think aggregate-number is more consistent with the > code, and cpu-count would look strange in the uncore case above where > the number of CPUs (really hyperthreads) is 72. Perhaps we should also > be outputting the aggregation mode with the number. Anyway, I think > for the patch series I'd prefer we skipped this one and kept the rest.
Right, I think we need a more general term to include non-cpu events. But it seems Arnaldo already merged it.
Arnaldo, do you want me to send a revert?
Thanks, Namhyung
| |