[14/15] perf stat: Rename "aggregate-number" to "cpu-count" in JSON

  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. :)

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
  

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.

Thanks,
Ian

> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/stat-display.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 43640115454c..7a39a1a7261d 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -281,19 +281,19 @@ static void print_aggr_id_json(struct perf_stat_config *config,
>
>         switch (config->aggr_mode) {
>         case AGGR_CORE:
> -               fprintf(output, "\"core\" : \"S%d-D%d-C%d\", \"aggregate-number\" : %d, ",
> +               fprintf(output, "\"core\" : \"S%d-D%d-C%d\", \"cpu-count\" : %d, ",
>                         id.socket, id.die, id.core, nr);
>                 break;
>         case AGGR_DIE:
> -               fprintf(output, "\"die\" : \"S%d-D%d\", \"aggregate-number\" : %d, ",
> +               fprintf(output, "\"die\" : \"S%d-D%d\", \"cpu-count\" : %d, ",
>                         id.socket, id.die, nr);
>                 break;
>         case AGGR_SOCKET:
> -               fprintf(output, "\"socket\" : \"S%d\", \"aggregate-number\" : %d, ",
> +               fprintf(output, "\"socket\" : \"S%d\", \"cpu-count\" : %d, ",
>                         id.socket, nr);
>                 break;
>         case AGGR_NODE:
> -               fprintf(output, "\"node\" : \"N%d\", \"aggregate-number\" : %d, ",
> +               fprintf(output, "\"node\" : \"N%d\", \"cpu-count\" : %d, ",
>                         id.node, nr);
>                 break;
>         case AGGR_NONE:
> --
> 2.38.1.584.g0f3c55d4c2-goog
>
  

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.

Thanks,
Namhyung
  

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.

Thanks,
Ian
> Namhyung
  

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
  

On Tue, Nov 29, 2022 at 2:46 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> 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

This is also breaking the json output test:

$ perf test -vv 89
89: perf stat JSON output linter                                    :
--- start ---
test child forked, pid 2116261
Checking json output: no args [Success]
Checking json output: system wide [Success]
Checking json output: interval [Success]
Checking json output: event [Success]
Checking json output: per thread [Success]
Checking json output: per node Test failed for input:
{"node" : "N0", "cpu-count" : 16, "counter-value" : "32.468431",
"unit" : "msec", "event" : "cpu-clo
ck", "event-runtime" : 32498339, "pcnt-running" : 100.00,
"metric-value" : 19.450525, "metric-unit"
: "CPUs utilized"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "52.000000",
"unit" : "", "event" : "context-swi
tches", "event-runtime" : 32471361, "pcnt-running" : 100.00,
"metric-value" : 1.601556, "metric-unit
" : "K/sec"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "16.000000",
"unit" : "", "event" : "cpu-migrati
ons", "event-runtime" : 32469950, "pcnt-running" : 100.00,
"metric-value" : 492.786362, "metric-unit
" : "/sec"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "57.000000",
"unit" : "", "event" : "page-faults
", "event-runtime" : 32474408, "pcnt-running" : 100.00, "metric-value"
: 1.755551, "metric-unit" : "
K/sec"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "2958499.000000",
"unit" : "", "event" : "cycles
", "event-runtime" : 32411643, "pcnt-running" : 100.00, "metric-value"
: 0.091119, "metric-unit" : "
GHz"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "3175893.000000",
"unit" : "", "event" : "instru
ctions", "event-runtime" : 32403573, "pcnt-running" : 100.00,
"metric-value" : 1.073481, "metric-uni
t" : "insn per cycle"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "688120.000000",
"unit" : "", "event" : "branche
s", "event-runtime" : 32391536, "pcnt-running" : 100.00,
"metric-value" : 21.193509, "metric-unit" :
"M/sec"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "17584.000000",
"unit" : "", "event" : "branch-m
isses", "event-runtime" : 32371972, "pcnt-running" : 100.00,
"metric-value" : 2.555368, "metric-unit
" : "of all branches"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "14377890.000000",
"unit" : "", "event" : "slots
", "event-runtime" : 32350026, "pcnt-running" : 100.00, "metric-value"
: 442.826757, "metric-unit" :
"M/sec"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "3380921.000000",
"unit" : "", "event" : "topdow
n-retiring", "event-runtime" : 32350026, "pcnt-running" : 100.00,
"metric-value" : 23.514767, "metri
c-unit" : "Retiring"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "1444174.000000",
"unit" : "", "event" : "topdow
n-bad-spec", "event-runtime" : 32350026, "pcnt-running" : 100.00,
"metric-value" : 10.044427, "metri
c-unit" : "Bad Speculation"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "5899393.000000",
"unit" : "", "event" : "topdow
n-fe-bound", "event-runtime" : 32350026, "pcnt-running" : 100.00,
"metric-value" : 41.031084, "metri
c-unit" : "Frontend Bound"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "3653375.000000",
"unit" : "", "event" : "topdow
n-be-bound", "event-runtime" : 32350026, "pcnt-running" : 100.00,
"metric-value" : 25.409722, "metri
c-unit" : "Backend Bound"}

Traceback (most recent call last):
 File "/home/irogers/kernel.org/./tools/perf/tests/shell/lib/perf_json_output_lint.py",
line 93, in
<module>
   check_json_output(expected_items)
 File "/home/irogers/kernel.org/./tools/perf/tests/shell/lib/perf_json_output_lint.py",
line 78, in
check_json_output
   raise RuntimeError(f'Unexpected key: key={key} value={value}')
RuntimeError: Unexpected key: key=cpu-count value=16
test child finished with -1
---- end ----
perf stat JSON output linter: FAILED!

Thanks,
Ian