<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 4, 2022, 12:06 AM Athira Rajeev <<a href="mailto:atrajeev@linux.vnet.ibm.com">atrajeev@linux.vnet.ibm.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
> On 04-Oct-2022, at 12:21 AM, Ian Rogers <<a href="mailto:irogers@google.com" target="_blank" rel="noreferrer">irogers@google.com</a>> wrote:<br>
> <br>
> On Mon, Oct 3, 2022 at 7:03 AM atrajeev <<a href="mailto:atrajeev@imap.linux.ibm.com" target="_blank" rel="noreferrer">atrajeev@imap.linux.ibm.com</a>> wrote:<br>
>> <br>
>> On 2022-10-02 05:17, Ian Rogers wrote:<br>
>>> On Thu, Sep 29, 2022 at 5:56 AM James Clark <<a href="mailto:james.clark@arm.com" target="_blank" rel="noreferrer">james.clark@arm.com</a>><br>
>>> wrote:<br>
>>>> <br>
>>>> <br>
>>>> <br>
>>>> On 29/09/2022 09:49, Athira Rajeev wrote:<br>
>>>>> <br>
>>>>> <br>
>>>>>> On 28-Sep-2022, at 9:05 PM, James Clark <<a href="mailto:james.clark@arm.com" target="_blank" rel="noreferrer">james.clark@arm.com</a>> wrote:<br>
>>>>>> <br>
>>>>>> <br>
>>>>>> <br>
>>>>> <br>
>>>>> Hi James,<br>
>>>>> <br>
>>>>> Thanks for looking at the patch and sharing review comments.<br>
>>>>> <br>
>>>>>> On 13/09/2022 12:57, Athira Rajeev wrote:<br>
>>>>>>> perf stat includes option to specify aggr_mode to display<br>
>>>>>>> per-socket, per-core, per-die, per-node counter details.<br>
>>>>>>> Also there is option -A ( AGGR_NONE, -no-aggr ), where the<br>
>>>>>>> counter values are displayed for each cpu along with "CPU"<br>
>>>>>>> value in one field of the output.<br>
>>>>>>> <br>
>>>>>>> Each of the aggregate mode uses the information fetched<br>
>>>>>>> from "/sys/devices/system/cpu/cpuX/topology" like core_id,<br>
>>>>>> <br>
>>>>>> I thought that this wouldn't apply to the cpu field because cpu is<br>
>>>>>> basically interchangeable as an index in cpumap, rather than anything<br>
>>>>>> being read from the topology file.<br>
>>>>> <br>
>>>>> The cpu value is filled in this function:<br>
>>>>> <br>
>>>>> Function : aggr_cpu_id__cpu<br>
>>>>> Code: util/cpumap.c<br>
>>>>> <br>
>>>>>> <br>
>>>>>>> physical_package_id. Utility functions in "cpumap.c" fetches<br>
>>>>>>> this information and populates the socket id, core id, cpu etc.<br>
>>>>>>> If the platform does not expose the topology information,<br>
>>>>>>> these values will be set to -1. Example, in case of powerpc,<br>
>>>>>>> details like physical_package_id is restricted to be exposed<br>
>>>>>>> in pSeries platform. So id.socket, id.core, id.cpu all will<br>
>>>>>>> be set as -1.<br>
>>>>>>> <br>
>>>>>>> In case of displaying socket or die value, there is no check<br>
>>>>>>> done in the "aggr_printout" function to see if it points to<br>
>>>>>>> valid socket id or die. But for displaying "cpu" value, there<br>
>>>>>>> is a check for "if (id.core > -1)". In case of powerpc pSeries<br>
>>>>>>> where detail like physical_package_id is restricted to be<br>
>>>>>>> exposed, id.core will be set to -1. Hence the column or field<br>
>>>>>>> itself for CPU won't be displayed in the output.<br>
>>>>>>> <br>
>>>>>>> Result for per-socket:<br>
>>>>>>> <br>
>>>>>>> <<>><br>
>>>>>>> perf stat -e branches --per-socket -a true<br>
>>>>>>> <br>
>>>>>>> Performance counter stats for 'system wide':<br>
>>>>>>> <br>
>>>>>>> S-1 32 416,851 branches<br>
>>>>>>> <<>><br>
>>>>>>> <br>
>>>>>>> Here S has -1 in above result. But with -A option which also<br>
>>>>>>> expects CPU in one column in the result, below is observed.<br>
>>>>>>> <br>
>>>>>>> <<>><br>
>>>>>>> /bin/perf stat -e instructions -A -a true<br>
>>>>>>> <br>
>>>>>>> Performance counter stats for 'system wide':<br>
>>>>>>> <br>
>>>>>>> 47,146 instructions<br>
>>>>>>> 45,226 instructions<br>
>>>>>>> 43,354 instructions<br>
>>>>>>> 45,184 instructions<br>
>>>>>>> <<>><br>
>>>>>>> <br>
>>>>>>> If the cpu id value is pointing to -1 also, it makes sense<br>
>>>>>>> to display the column in the output to replicate the behaviour<br>
>>>>>>> or to be in precedence with other aggr options(like per-socket,<br>
>>>>>>> per-core). Remove the check "id.core" so that CPU field gets<br>
>>>>>>> displayed in the output.<br>
>>>>>> <br>
>>>>>> Why would you want to print -1 out? Seems like the if statement was a<br>
>>>>>> good one to me, otherwise the output looks a bit broken to users. Are<br>
>>>>>> the other aggregation modes even working if -1 is set for socket and<br>
>>>>>> die? Maybe we need to not print -1 in those cases or exit earlier with a<br>
>>>>>> failure.<br>
>>>>>> <br>
>>>>>> The -1 value has a specific internal meaning which is "to not<br>
>>>>>> aggregate". It doesn't mean "not set".<br>
>>>>> <br>
>>>>> Currently, this check is done only for printing cpu value.<br>
>>>>> For socket/die/core values, this check is not done. Pasting an<br>
>>>>> example snippet from a powerpc system ( specifically from pseries platform where<br>
>>>>> the value is set to -1 )<br>
>>>>> <br>
>>>>> ./perf stat --per-core -a -C 1 true<br>
>>>>> <br>
>>>>> Performance counter stats for 'system wide':<br>
>>>>> <br>
>>>>> S-1-D-1-C-1 1 1.06 msec cpu-clock # 1.018 CPUs utilized<br>
>>>>> S-1-D-1-C-1 1 2 context-switches # 1.879 K/sec<br>
>>>>> S-1-D-1-C-1 1 0 cpu-migrations # 0.000 /sec<br>
>>>>> <br>
>>>>> Here though the value is -1, we are displaying it. Where as in case of cpu, the first column will be<br>
>>>>> empty since we do a check before printing.<br>
>>>>> <br>
>>>>> Example:<br>
>>>>> <br>
>>>>> ./perf stat --per-core -A -C 1 true<br>
>>>>> <br>
>>>>> Performance counter stats for 'CPU(s) 1':<br>
>>>>> <br>
>>>>> 0.88 msec cpu-clock # 1.022 CPUs utilized<br>
>>>>> 2 context-switches<br>
>>>>> 0 cpu-migrations<br>
>>>>> <br>
>>>>> <br>
>>>>> No sure, whether there are scripts out there, which consume the current format and<br>
>>>>> not displaying -1 may break it. That is why we tried with change to remove check for cpu, similar to<br>
>>>>> other modes like socket, die, core etc.<br>
>>>> <br>
>>>> I wouldn't worry about that because there are json and CSV modes which<br>
>>>> are machine readable, and -1 is already not always displayed. If<br>
>>>> anything this change here is also likely to break parsing by adding -1<br>
>>>> where it wasn't before.<br>
>>>> <br>
>>>>> <br>
>>>>> Also perf code ie “aggr_cpu_id__empty” in util/cpumap.c initialises the<br>
>>>>> values to -1 . I was checking to see where we are mapping -1 to “to not aggregate”.<br>
>>>>> What I could find is AGGR_NONE ( which is for no-aggr ) has value as zero.<br>
>>>>> <br>
>>>>> Reference: defined in util/stat.h<br>
>>>>> <br>
>>>>> enum aggr_mode {<br>
>>>>> AGGR_NONE,<br>
>>>>> <br>
>>>> <br>
>>>> That enum is never written to any of the cpumap members, that defines<br>
>>>> the mode of how to fill the cpu map instead. 0 is a valid value, for<br>
>>>> example "CPU 0". -1 is used as a special case and shouldn't be<br>
>>>> displayed<br>
>>>> IMO.<br>
>>>> <br>
>>>> Did you see my comment in the code below about the bad merge? Could<br>
>>>> that<br>
>>>> not be related to your issue?<br>
>>> <br>
>>> I'm suspicious of this too. In Claire's patch:<br>
>>> <br>
>>> case AGGR_NONE:<br>
>>> - if (evsel->percore && !config->percore_show_thread) {<br>
>>> - fprintf(config->output, "S%d-D%d-C%*d%s",<br>
>>> - id.socket,<br>
>>> - id.die,<br>
>>> - config->csv_output ? 0 : -3,<br>
>>> - id.core, config->csv_sep);<br>
>>> - } else if (id.cpu.cpu > -1) {<br>
>>> - fprintf(config->output, "CPU%*d%s",<br>
>>> - config->csv_output ? 0 : -7,<br>
>>> - id.cpu.cpu, config->csv_sep);<br>
>>> + if (config->json_output) {<br>
>>> + if (evsel->percore &&<br>
>>> !config->percore_show_thread) {<br>
>>> + fprintf(config->output, "\"core\" :<br>
>>> \"S%d-D%d-C%d\"",<br>
>>> + id.socket,<br>
>>> + id.die,<br>
>>> + id.core);<br>
>>> + } else if (id.core > -1) {<br>
>>> + fprintf(config->output, "\"cpu\" :<br>
>>> \"%d\", ",<br>
>>> + id.cpu.cpu);<br>
>>> + }<br>
>>> + } else {<br>
>>> + if (evsel->percore &&<br>
>>> !config->percore_show_thread) {<br>
>>> + fprintf(config->output,<br>
>>> "S%d-D%d-C%*d%s",<br>
>>> + id.socket,<br>
>>> + id.die,<br>
>>> + config->csv_output ? 0 : -3,<br>
>>> + id.core, config->csv_sep);<br>
>>> + } else if (id.core > -1) {<br>
>>> + fprintf(config->output, "CPU%*d%s",<br>
>>> + config->csv_output ? 0 : -7,<br>
>>> + id.cpu.cpu, config->csv_sep);<br>
>>> + }<br>
>>> }<br>
>>> break;<br>
>>> <br>
>>> The old code was using "id.cpu.cpu > -1" while the new code is<br>
>>> "id.core > -1". The value printed is id.cpu.cpu and so testing id.core<br>
>>> makes less sense to me. Going back to the original patch:<br>
>>> <br>
>>> <a href="https://lore.kernel.org/lkml/20210811224317.1811618-1-cjense@google.com/" rel="noreferrer noreferrer" target="_blank">https://lore.kernel.org/lkml/20210811224317.1811618-1-cjense@google.com/</a><br>
>>> case AGGR_NONE:<br>
>>> - if (evsel->percore && !config->percore_show_thread) {<br>
>>> - fprintf(config->output, "S%d-D%d-C%*d%s",<br>
>>> - id.socket,<br>
>>> - id.die,<br>
>>> - config->csv_output ? 0 : -3,<br>
>>> - id.core, config->csv_sep);<br>
>>> + if (config->json_output) {<br>
>>> + if (evsel->percore && !config->percore_show_thread) {<br>
>>> + fprintf(config->output, "\"core\" : \"S%d-D%d-C%d\"",<br>
>>> + id.socket,<br>
>>> + id.die,<br>
>>> + id.core);<br>
>>> + } else if (id.core > -1) {<br>
>>> + fprintf(config->output, "\"cpu\" : \"%d\", ",<br>
>>> + evsel__cpus(evsel)->map[id.core]);<br>
>>> + }<br>
>>> + } else {<br>
>>> + if (evsel->percore && !config->percore_show_thread) {<br>
>>> + fprintf(config->output, "S%d-D%d-C%*d%s",<br>
>>> + id.socket,<br>
>>> + id.die,<br>
>>> + config->csv_output ? 0 : -3,<br>
>>> + id.core, config->csv_sep);<br>
>>> } else if (id.core > -1) {<br>
>>> fprintf(config->output, "CPU%*d%s",<br>
>>> config->csv_output ? 0 : -7,<br>
>>> evsel__cpus(evsel)->map[id.core],<br>
>>> config->csv_sep);<br>
>>> - }<br>
>>> + }<br>
>>> + }<br>
>>> +<br>
>>> break;<br>
>>> <br>
>>> So testing the id.core isn't a bad index makes sense. However, we<br>
>>> changed from core to CPU here:<br>
>>> <a href="https://lore.kernel.org/all/20220105061351.120843-26-irogers@google.com/" rel="noreferrer noreferrer" target="_blank">https://lore.kernel.org/all/20220105061351.120843-26-irogers@google.com/</a><br>
>>> and that was because of:<br>
>>> <a href="https://lore.kernel.org/r/20220105061351.120843-25-irogers@google.com" rel="noreferrer noreferrer" target="_blank">https://lore.kernel.org/r/20220105061351.120843-25-irogers@google.com</a><br>
>>> <br>
>>> So I think the code needs to test CPU and not core. Whether that is<br>
>>> addressing the Power test failures is another matter, as James said we<br>
>>> may need a fix in the tests for that.<br>
>>> <br>
>> <br>
>> Hi Ian, James<br>
>> <br>
>> Thanks for the reviews and suggestions.<br>
>> <br>
>> After checking through the original commits for id.core vs cpu check,<br>
>> sharing patch below to test CPU and not core.<br>
>> <br>
>> From 4dd98d953940deb2f85176cb6b4ecbfd18dbdbf9 Mon Sep 17 00:00:00 2001<br>
>> From: Athira Rajeev <<a href="mailto:atrajeev@linux.vnet.ibm.com" target="_blank" rel="noreferrer">atrajeev@linux.vnet.ibm.com</a>><br>
>> Date: Mon, 3 Oct 2022 15:47:27 +0530<br>
>> Subject: [PATCH] tools/perf: Fix cpu check to use id.cpu.cpu in<br>
>> aggr_printout<br>
>> <br>
>> perf stat has options to aggregate the counts in different<br>
>> modes like per socket, per core etc. The function "aggr_printout"<br>
>> in util/stat-display.c which is used to print the aggregates,<br>
>> has a check for cpu in case of AGGR_NONE. This check was<br>
>> originally using condition : "if (id.cpu.cpu > -1)". But<br>
>> this got changed after commit df936cadfb58 ("perf stat: Add<br>
>> JSON output option"), which added option to output json format<br>
>> for different aggregation modes. After this commit, the<br>
>> check in "aggr_printout" is using "if (id.core > -1)".<br>
>> <br>
>> The old code was using "id.cpu.cpu > -1" while the new code<br>
>> is using "id.core > -1". But since the value printed is<br>
>> id.cpu.cpu, fix this check to use cpu and not core.<br>
>> <br>
>> Signed-off-by: Athira Rajeev <<a href="mailto:atrajeev@linux.vnet.ibm.com" target="_blank" rel="noreferrer">atrajeev@linux.vnet.ibm.com</a>><br>
>> Suggested-by: James Clark <<a href="mailto:james.clark@arm.com" target="_blank" rel="noreferrer">james.clark@arm.com</a>><br>
>> Suggested-by: Ian Rogers <<a href="mailto:irogers@google.com" target="_blank" rel="noreferrer">irogers@google.com</a>><br>
> <br>
> The change below works on my dual socket SkylakeX:<br>
> ..<br>
> 85: perf stat CSV output linter :<br>
> Ok<br>
> 86: perf stat csv summary test : Ok<br>
> 87: perf stat JSON output linter : Ok<br>
> ..<br>
> I don't see anything else out of the ordinary.<br>
> <br>
> Thanks,<br>
> Ian<br>
> <br>
<br>
Hi Ian,<br>
Thanks for helping with testing. Can I add your Tested-by for the patch ?<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Yep.</div><div dir="auto"><br></div><div dir="auto">Tested-by: Ian Rogers <<a href="mailto:irogers@google.com">irogers@google.com</a>></div><div dir="auto"><br></div><div dir="auto">Thanks,</div><div dir="auto">Ian</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Arnaldo,<br>
Please suggest if I have to send as separate patch for the cpu check fix patch pasted above:<br>
"tools/perf: Fix cpu check to use id.cpu.cpu in aggr_printout”<br>
<br>
Thanks<br>
Athira<br>
>> ---<br>
>> tools/perf/util/stat-display.c | 4 ++--<br>
>> 1 file changed, 2 insertions(+), 2 deletions(-)<br>
>> <br>
>> diff --git a/tools/perf/util/stat-display.c<br>
>> b/tools/perf/util/stat-display.c<br>
>> index b82844cb0ce7..cf28020798ec 100644<br>
>> --- a/tools/perf/util/stat-display.c<br>
>> +++ b/tools/perf/util/stat-display.c<br>
>> @@ -168,7 +168,7 @@ static void aggr_printout(struct perf_stat_config<br>
>> *config,<br>
>> id.socket,<br>
>> id.die,<br>
>> id.core);<br>
>> - } else if (id.core > -1) {<br>
>> + } else if (id.cpu.cpu > -1) {<br>
>> fprintf(config->output, "\"cpu\" : \"%d\", ",<br>
>> id.cpu.cpu);<br>
>> }<br>
>> @@ -179,7 +179,7 @@ static void aggr_printout(struct perf_stat_config<br>
>> *config,<br>
>> id.die,<br>
>> config->csv_output ? 0 : -3,<br>
>> id.core, config->csv_sep);<br>
>> - } else if (id.core > -1) {<br>
>> + } else if (id.cpu.cpu > -1) {<br>
>> fprintf(config->output, "CPU%*d%s",<br>
>> config->csv_output ? 0 : -7,<br>
>> id.cpu.cpu, config->csv_sep);<br>
>> --<br>
>> 2.31.1<br>
>> <br>
>> Can you suggest or help to test this patch change.<br>
>> <br>
>> To address the test failure, as James suggested, I will handle fix in<br>
>> testcases and post them<br>
>> as a separate patch. Plan is to add a sanity check in the tests to see<br>
>> if the "physical_packagge_id" ( ie socket id ) in topology points to -1<br>
>> and if so skip the test. Also in parallel, checking to see how we can<br>
>> handle the aggregation modes to work incase of "-1" value for socket or<br>
>> die<br>
>> <br>
>> Thanks<br>
>> Athira<br>
>> <br>
>>> Thanks,<br>
>>> Ian<br>
>>> <br>
>>>> Or the one about fixing it in the test instead? Or failing early if<br>
>>>> the<br>
>>>> topology can't be read?<br>
>>>> <br>
>>>> I'm still not convinced that any of the modes where -1 is printed are<br>
>>>> even working properly so it might be best to fix that rather than just<br>
>>>> the printout.<br>
>>>> <br>
>>>>> James, can you point me to reference for that meaning if I have missed anything.<br>
>>>> <br>
>>>> It's here:<br>
>>>> <br>
>>>> /** Identify where counts are aggregated, -1 implies not to<br>
>>>> aggregate. */<br>
>>>> struct aggr_cpu_id {<br>
>>>> <br>
>>>>> <br>
>>>>> Thanks<br>
>>>>> Athira<br>
>>>>> <br>
>>>>>> <br>
>>>>>>> <br>
>>>>>>> After the fix:<br>
>>>>>>> <br>
>>>>>>> <<>><br>
>>>>>>> perf stat -e instructions -A -a true<br>
>>>>>>> <br>
>>>>>>> Performance counter stats for 'system wide':<br>
>>>>>>> <br>
>>>>>>> CPU-1 64,034 instructions<br>
>>>>>>> CPU-1 68,941 instructions<br>
>>>>>>> CPU-1 59,418 instructions<br>
>>>>>>> CPU-1 70,478 instructions<br>
>>>>>>> CPU-1 65,201 instructions<br>
>>>>>>> CPU-1 63,704 instructions<br>
>>>>>>> <<>><br>
>>>>>>> <br>
>>>>>>> This is caught while running "perf test" for<br>
>>>>>>> "stat+json_output.sh" and "stat+csv_output.sh".<br>
>>>>>> <br>
>>>>>> Is it possible to fix the issue by making the tests cope with the lack<br>
>>>>>> of the CPU id?<br>
>>>>>> <br>
>>>>>>> <br>
>>>>>>> Reported-by: Disha Goel <<a href="mailto:disgoel@linux.vnet.ibm.com" target="_blank" rel="noreferrer">disgoel@linux.vnet.ibm.com</a>><br>
>>>>>>> Signed-off-by: Athira Rajeev <<a href="mailto:atrajeev@linux.vnet.ibm.com" target="_blank" rel="noreferrer">atrajeev@linux.vnet.ibm.com</a>><br>
>>>>>>> ---<br>
>>>>>>> tools/perf/util/stat-display.c | 6 ++----<br>
>>>>>>> 1 file changed, 2 insertions(+), 4 deletions(-)<br>
>>>>>>> <br>
>>>>>>> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c<br>
>>>>>>> index b82844cb0ce7..1b751a730271 100644<br>
>>>>>>> --- a/tools/perf/util/stat-display.c<br>
>>>>>>> +++ b/tools/perf/util/stat-display.c<br>
>>>>>>> @@ -168,10 +168,9 @@ static void aggr_printout(struct perf_stat_config *config,<br>
>>>>>>> id.socket,<br>
>>>>>>> id.die,<br>
>>>>>>> id.core);<br>
>>>>>>> - } else if (id.core > -1) {<br>
>>>>>>> + } else<br>
>>>>>> <br>
>>>>>> This should have been "id.cpu.cpu > -1". Looks like it was changed by<br>
>>>>>> some kind of bad merge or rebase in df936cadfb because there is no<br>
>>>>>> obvious justification for the change to .core in that commit.<br>
>>>>> <br>
>>>>>> <br>
>>>>>>> fprintf(config->output, "\"cpu\" : \"%d\", ",<br>
>>>>>>> id.cpu.cpu);<br>
>>>>>>> - }<br>
>>>>>>> } else {<br>
>>>>>>> if (evsel->percore && !config->percore_show_thread) {<br>
>>>>>>> fprintf(config->output, "S%d-D%d-C%*d%s",<br>
>>>>>>> @@ -179,11 +178,10 @@ static void aggr_printout(struct perf_stat_config *config,<br>
>>>>>>> id.die,<br>
>>>>>>> config->csv_output ? 0 : -3,<br>
>>>>>>> id.core, config->csv_sep);<br>
>>>>>>> - } else if (id.core > -1) {<br>
>>>>>>> + } else<br>
>>>>>>> fprintf(config->output, "CPU%*d%s",<br>
>>>>>>> config->csv_output ? 0 : -7,<br>
>>>>>>> id.cpu.cpu, config->csv_sep);<br>
>>>>>>> - }<br>
>>>>>>> }<br>
>>>>>>> break;<br>
>>>>>>> case AGGR_THREAD:<br>
>>>>> <br>
<br>
</blockquote></div></div></div>