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.
