Re: [PATCH] perf list: Add s390 support for detailed/verbose pmu event description

[Thomas-Mich Richter]

On 04/09/2018 04:18 PM, Mark Rutland wrote:
> On Mon, Apr 09, 2018 at 03:03:32PM +0200, Thomas-Mich Richter wrote:
>> On 04/09/2018 01:37 PM, Mark Rutland wrote:
>>> On Mon, Apr 09, 2018 at 01:00:15PM +0200, Thomas Richter wrote:
>>>> @@ -562,6 +562,12 @@ static int is_pmu_core(const char *name)
>>>>  	if (stat(path, &st) == 0)
>>>>  		return 1;
>>>>  
>>>> +	/* Look for cpu sysfs (specific to s390) */
>>>> +	scnprintf(path, PATH_MAX, "%s/bus/event_source/devices/%s",
>>>> +		  sysfs, name);
>>>> +	if (stat(path, &st) == 0)
>>>> +		return 1;
>>>
>>> IIUC here we return true if the PMU has a sysfs directory, but all PMUs
>>> (including uncore PMUs) should have such a directory, so this will make
>>> is_pmu_core() always return true.
>>>
>>> Can you match the "cpum_" prefix specifically, instead?
>>>
>>> Thanks,
>>> Mark.
>>
>> I am sorry, I don't follow you.
>>
>> When I look at the code function sequence
>>
>> perf_pmu__scan()
>> +---> pmu_read_sysfs()
>>       This functions scans directory /sys/bus/event_source/devices/
>>       and calls for each entry in this directory. For s390 these entries exist:
>>       cpum_sf cpum_cf tracepoint and software
> 
> ... and we want is:
> 
> 	is_pmu_core("cpum_sf") == true
> 	is_pmu_core("cpum_cf") == true
> 	is_pmu_core("tracepoint") == false
> 	is_pmu_core("software") == false
> 
>>       +---> perf_pmu__find();
>>             +---> pmu_lookup()
> 
>>                   +---> pmu_add_cpu_aliases() adds the list of aliases such as cpum_sf/SF_CYCLES_BASIC/
>>                         to the global list of aliases. But ths happens only when
>>                         +---> is_pmu_core()
>>                               function returns true.
>>                               And for s390 it needs to test for /sys/bus/event_source/devices/cpum_sf/ and
>>                               /sys/bus/event_source/devices/cpum_cf/
>>                               directories.
>>                               These directories are used to read the alias names in function
>>                               pmu_aliases() above.
> 
> However, your code also returns true for "tracepoint" and "software".
> 
> You check if there's a directory under event_source/devices/ for the PMU
> name, and every PMU should have such a directory.
> 
> For example, on my x86 box I have
> 
> [mark@lakrids:~]% ls /sys/bus/event_source/devices 
> breakpoint   power           uncore_cbox_13  uncore_cbox_9  uncore_qpi_0
> cpu          software        uncore_cbox_2   uncore_ha_0    uncore_qpi_1
> cstate_core  tracepoint      uncore_cbox_3   uncore_ha_1    uncore_r2pcie
> cstate_pkg   uncore_cbox_0   uncore_cbox_4   uncore_imc_0   uncore_r3qpi_0
> intel_bts    uncore_cbox_1   uncore_cbox_5   uncore_imc_1   uncore_r3qpi_1
> intel_cqm    uncore_cbox_10  uncore_cbox_6   uncore_imc_4   uncore_ubox
> intel_pt     uncore_cbox_11  uncore_cbox_7   uncore_imc_5
> msr          uncore_cbox_12  uncore_cbox_8   uncore_pcu
> 
> 
> ... and with your patch and the below hack applied:
> 
> ----
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index bd0a32b03523..cec6bf551fe3 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -675,6 +675,12 @@ static void pmu_add_cpu_aliases(struct list_head *head, struct perf_pmu *pmu)
>                         break;
>                 }
>  
> +               if (is_pmu_core(name)) {
> +                       printf ("is_pmu_core(\"%s\") is true\n", name);
> +               } else {
> +                       printf ("is_pmu_core(\"%s\") is false\n", name);
> +               }
> +
>                 if (!is_pmu_core(name)) {
>                         /* check for uncore devices */
>                         if (pe->pmu == NULL)
> ----
> 
> ... is_pmu_core() returns true for every PMU:
> 
> [mark@lakrids:~/src/linux/tools/perf]% ./perf list | grep 'is true' | uniq
> is_pmu_core("uncore_imc_4") is true
> is_pmu_core("uncore_cbox_5") is true
> is_pmu_core("uncore_ha_0") is true
> is_pmu_core("uncore_cbox_3") is true
> is_pmu_core("uncore_qpi_0") is true
> is_pmu_core("cstate_pkg") is true
> is_pmu_core("breakpoint") is true
> is_pmu_core("uncore_imc_0") is true
> is_pmu_core("uncore_ubox") is true
> is_pmu_core("uncore_pcu") is true
> is_pmu_core("uncore_cbox_1") is true
> is_pmu_core("uncore_r3qpi_0") is true
> is_pmu_core("uncore_cbox_13") is true
> is_pmu_core("intel_cqm") is true
> is_pmu_core("power") is true
> is_pmu_core("cpu") is true
> is_pmu_core("intel_pt") is true
> is_pmu_core("uncore_cbox_11") is true
> is_pmu_core("uncore_cbox_8") is true
> is_pmu_core("uncore_imc_5") is true
> is_pmu_core("software") is true
> is_pmu_core("uncore_cbox_6") is true
> is_pmu_core("uncore_ha_1") is true
> is_pmu_core("uncore_r2pcie") is true
> is_pmu_core("uncore_cbox_4") is true
> is_pmu_core("intel_bts") is true
> is_pmu_core("uncore_qpi_1") is true
> is_pmu_core("uncore_imc_1") is true
> is_pmu_core("uncore_cbox_2") is true
> is_pmu_core("uncore_r3qpi_1") is true
> is_pmu_core("uncore_cbox_0") is true
> is_pmu_core("cstate_core") is true
> is_pmu_core("uncore_cbox_12") is true
> is_pmu_core("tracepoint") is true
> is_pmu_core("uncore_cbox_9") is true
> is_pmu_core("msr") is true
> is_pmu_core("uncore_cbox_10") is true
> is_pmu_core("uncore_cbox_7") is true
> 
> [mark@lakrids:~/src/linux/tools/perf]% ./perf list | grep 'is false' | wc -l
> 0
> 
> 
> ... whereas previously this was only the case for the CPU PMU:
> 
> [mark@lakrids:~/src/linux/tools/perf]% ./perf list | grep 'is true' | uniq
> is_pmu_core("cpu") is true
> 
> [mark@lakrids:~/src/linux/tools/perf]% ./perf list | grep 'is false' | uniq
> is_pmu_core("uncore_imc_4") is false
> is_pmu_core("uncore_cbox_5") is false
> is_pmu_core("uncore_ha_0") is false
> is_pmu_core("uncore_cbox_3") is false
> is_pmu_core("uncore_qpi_0") is false
> is_pmu_core("cstate_pkg") is false
> is_pmu_core("breakpoint") is false
> is_pmu_core("uncore_imc_0") is false
> is_pmu_core("uncore_ubox") is false
> is_pmu_core("uncore_pcu") is false
> is_pmu_core("uncore_cbox_1") is false
> is_pmu_core("uncore_r3qpi_0") is false
> is_pmu_core("uncore_cbox_13") is false
> is_pmu_core("intel_cqm") is false
> is_pmu_core("power") is false
> is_pmu_core("intel_pt") is false
> is_pmu_core("uncore_cbox_11") is false
> is_pmu_core("uncore_cbox_8") is false
> is_pmu_core("uncore_imc_5") is false
> is_pmu_core("software") is false
> is_pmu_core("uncore_cbox_6") is false
> is_pmu_core("uncore_ha_1") is false
> is_pmu_core("uncore_r2pcie") is false
> is_pmu_core("uncore_cbox_4") is false
> is_pmu_core("intel_bts") is false
> is_pmu_core("uncore_qpi_1") is false
> is_pmu_core("uncore_imc_1") is false
> is_pmu_core("uncore_cbox_2") is false
> is_pmu_core("uncore_r3qpi_1") is false
> is_pmu_core("uncore_cbox_0") is false
> is_pmu_core("cstate_core") is false
> is_pmu_core("uncore_cbox_12") is false
> is_pmu_core("tracepoint") is false
> is_pmu_core("uncore_cbox_9") is false
> is_pmu_core("msr") is false
> is_pmu_core("uncore_cbox_10") is false
> is_pmu_core("uncore_cbox_7") is false
> 
> If it's fine to have a tautological is_pmu_core(), then we can remove it
> entirely.
> 
> My understanding was that we have the is_pmu_core() check to ensure that
> we don't associate aliases with other PMUs in the system.
> 
> For example, on some arm platforms the CPU PMU isn't available, and we
> shouldn't add the CPU PMU aliases just because we have a software PMU.
> 
>> Maybe I misunderstand this whole scheme, but with this patch the perf
>> list commands works...
> 
> It looks like it works on my x86 box, at least, but I do think this is
> wrong in some cases.
> 
> Thanks,
> Mark.
> 

Thank you very much for your explanation. I think I got your point
and will provide a reworked patch which returns ok for the s390
PMUs named cpum_cf and cpum_sf.

-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
GeschÃftsfÃhrung: Dirk Wittkopp
Sitz der Gesellschaft: BÃblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294