Re: [PATCH] tools/perf: Fix aggr_printout to display cpu field irrespective of core value

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

> On 16-Sep-2022, at 5:01 PM, Disha Goel <disgoel@linux.ibm.com> wrote:
> 
> This Message Is From an External Sender
> This message came from outside your organization.
> 
> 
> On 9/13/22 5:27 PM, Athira Rajeev wrote:
>> perf stat includes option to specify aggr_mode to display
>> per-socket, per-core, per-die, per-node counter details.
>> Also there is option -A ( AGGR_NONE, -no-aggr ), where the
>> counter values are displayed for each cpu along with "CPU"
>> value in one field of the output.
>> 
>> Each of the aggregate mode uses the information fetched
>> from "/sys/devices/system/cpu/cpuX/topology" like core_id,
>> physical_package_id. Utility functions in "cpumap.c" fetches
>> this information and populates the socket id, core id, cpu etc.
>> If the platform does not expose the topology information,
>> these values will be set to -1. Example, in case of powerpc,
>> details like physical_package_id is restricted to be exposed
>> in pSeries platform. So id.socket, id.core, id.cpu all will
>> be set as -1.
>> 
>> In case of displaying socket or die value, there is no check
>> done in the "aggr_printout" function to see if it points to
>> valid socket id or die. But for displaying "cpu" value, there
>> is a check for "if (id.core > -1)". In case of powerpc pSeries
>> where detail like physical_package_id is restricted to be
>> exposed, id.core will be set to -1. Hence the column or field
>> itself for CPU won't be displayed in the output.
>> 
>> Result for per-socket:
>> 
>> <<>>
>> perf stat -e branches --per-socket -a true
>> 
>>  Performance counter stats for 'system wide':
>> 
>> S-1      32            416,851      branches
>> <<>>
>> 
>> Here S has -1 in above result. But with -A option which also
>> expects CPU in one column in the result, below is observed.
>> 
>> <<>>
>>  /bin/perf stat -e instructions -A -a true
>> 
>>  Performance counter stats for 'system wide':
>> 
>>             47,146      instructions
>>             45,226      instructions
>>             43,354      instructions
>>             45,184      instructions
>> <<>>
>> 
>> If the cpu id value is pointing to -1 also, it makes sense
>> to display the column in the output to replicate the behaviour
>> or to be in precedence with other aggr options(like per-socket,
>> per-core). Remove the check "id.core" so that CPU field gets
>> displayed in the output.
>> 
>> After the fix:
>> 
>> <<>>
>> perf stat -e instructions -A -a true
>> 
>>  Performance counter stats for 'system wide':
>> 
>> CPU-1                  64,034      instructions
>> CPU-1                  68,941      instructions
>> CPU-1                  59,418      instructions
>> CPU-1                  70,478      instructions
>> CPU-1                  65,201      instructions
>> CPU-1                  63,704      instructions
>> <<>>
>> 
>> This is caught while running "perf test" for
>> "stat+json_output.sh" and "stat+csv_output.sh".
>> 
>> Reported-by: Disha Goel 
>> <disgoel@linux.vnet.ibm.com>
>> 
>> Signed-off-by: Athira Rajeev 
>> <atrajeev@linux.vnet.ibm.com>
> Tested-by: Disha Goel <disgoel@linux.vnet.ibm.com>

Hi,

Looking for review comments for this change.

Thanks
Athira
>> ---
>>  tools/perf/util/stat-display.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
>> index b82844cb0ce7..1b751a730271 100644
>> --- a/tools/perf/util/stat-display.c
>> +++ b/tools/perf/util/stat-display.c
>> @@ -168,10 +168,9 @@ static void aggr_printout(struct perf_stat_config *config,
>>  					id.socket,
>>  					id.die,
>>  					id.core);
>> -			} else if (id.core > -1) {
>> +			} else
>>  				fprintf(config->output, "\"cpu\" : \"%d\", ",
>>  					id.cpu.cpu);
>> -			}
>>  		} else {
>>  			if (evsel->percore && !config->percore_show_thread) {
>>  				fprintf(config->output, "S%d-D%d-C%*d%s",
>> @@ -179,11 +178,10 @@ static void aggr_printout(struct perf_stat_config *config,
>>  					id.die,
>>  					config->csv_output ? 0 : -3,
>>  					id.core, config->csv_sep);
>> -			} else if (id.core > -1) {
>> +			} else
>>  				fprintf(config->output, "CPU%*d%s",
>>  					config->csv_output ? 0 : -7,
>>  					id.cpu.cpu, config->csv_sep);
>> -			}
>>  		}
>>  		break;
>>  	case AGGR_THREAD:
>>