Re: [PATCH v9 3/5] perf utils: use pmu->is_uncore to detect PMU UNCORE devices

From: Arnaldo Carvalho de Melo
Date: Tue Dec 05 2017 - 08:58:46 EST

Em Tue, Dec 05, 2017 at 12:42:30PM +0530, Ganapatrao Kulkarni escreveu:
> thanks Jin Yao for point this out.
>
> looks like logic of leveraging uncore device type(which i have changed
> in v9) does not go well
> with some json events of x86.
> can you please try below diff(logic used till v8), which keeps the
> original logic of identifying core/cpu PMUs.

This seems space mangled :-\

- Arnaldo

>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 5ad8a18..57e38fd 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -538,6 +538,34 @@ static bool pmu_is_uncore(const char *name)
> }
>
> /*
> + * PMU CORE devices have different name other than cpu in sysfs on some
> + * platforms. looking for possible sysfs files to identify as core device.
> + */
> +static int is_pmu_core(const char *name)
> +{
> + struct stat st;
> + char path[PATH_MAX];
> + const char *sysfs = sysfs__mountpoint();
> +
> + if (!sysfs)
> + return 0;
> +
> + /* Look for cpu sysfs (x86 and others) */
> + scnprintf(path, PATH_MAX, "%s/bus/event_source/devices/cpu", sysfs);
> + if ((stat(path, &st) == 0) &&
> + (strncmp(name, "cpu", strlen("cpu")) == 0))
> + return 1;
> +
> + /* Look for cpu sysfs (specific to arm) */
> + scnprintf(path, PATH_MAX, "%s/bus/event_source/devices/%s/cpus",
> + sysfs, name);
> + if (stat(path, &st) == 0)
> + return 1;
> +
> + return 0;
> +}
> +
> +/*
> * Return the CPU id as a raw string.
> *
> * Each architecture should provide a more precise id string that
> @@ -641,7 +669,7 @@ static void pmu_add_cpu_aliases(struct list_head
> *head, struct perf_pmu *pmu)
> break;
> }
>
> - if (pmu->is_uncore) {
> + if (!is_pmu_core(name)) {
> /* check for uncore devices */
> if (pe->pmu == NULL)
> continue;
>
> i have tried this diff on my x86 PC(haswell) and looks to be ok.
> please confirm your testing on skylake.
>
> gkulkarni@gkFc25>perf>> ./perf stat --per-thread -p 12663 -M CPI,IPC sleep 1
>
> Performance counter stats for process id '12663':
>
> bash-12663 278,886 inst_retired.any:u
> bash-12663 482,284 cycles:u
> bash-12663 278,886 inst_retired.any:u
> bash-12663 483,597
> cpu_clk_unhalted.thread:u
>
> 1.000923760 seconds time elapsed
>
>
> On Tue, Dec 5, 2017 at 7:42 AM, Jin, Yao <yao.jin@xxxxxxxxxxxxxxx> wrote:
> > Hi Kulkarni, Arnaldo,
> >
> > This patch has been merged in perf/core branch today.
> >
> > But I see a regression issue when I run the 'perf stat'.
> >
> > With bisect checking, I locate to this patch.
> >
> > commit ad8737a08973f5dca632bdd63cf2abc99670e540
> > Author: Ganapatrao Kulkarni <ganapatrao.kulkarni@xxxxxxxxxx>
> > Date: Tue Oct 17 00:02:20 2017 +0530
> >
> > perf pmu: Use pmu->is_uncore to detect UNCORE devices
> >
> > For example (on Intel skylake desktop),
> >
> > 1. The correct output should be (without this patch):
> >
> > root@skl:/tmp# perf stat --per-thread -p 1754 -M CPI,IPC
> > ^C
> > Performance counter stats for process id '1754':
> >
> > vmstat-1754 1,882,798 inst_retired.any #
> > 0.8 CPI
> > vmstat-1754 1,589,720 cycles
> > vmstat-1754 1,882,798 inst_retired.any #
> > 1.2 IPC
> > vmstat-1754 1,589,720 cpu_clk_unhalted.thread
> >
> > 2.647443167 seconds time elapsed
> >
> > 2. With this patch, the output will be:
> >
> > root@skl:/tmp# perf stat --per-thread -p 1754 -M CPI,IPC
> > ^C
> > Performance counter stats for process id '1754':
> >
> > vmstat-1754 1,945,589 inst_retired.any
> > vmstat-1754 <not supported> inst_retired.any
> > vmstat-1754 1,609,892 cycles
> > vmstat-1754 1,945,589 inst_retired.any
> > vmstat-1754 <not supported> inst_retired.any
> > vmstat-1754 1,609,892 cpu_clk_unhalted.thread
> > vmstat-1754 <not supported> cpu_clk_unhalted.thread
> >
> > 3.051274166 seconds time elapsed
> >
> > Could you please help to take a look?
> >
> > Thanks
> > Jin Yao
> >
> >
> > On 10/17/2017 2:32 AM, Ganapatrao Kulkarni wrote:
> >>
> >> PMU CORE devices are identified using sysfs filename cpu, however
> >> on some platforms(like arm/arm64), PMU CORE sysfs name is not cpu.
> >> Hence cpu cannot be used to differentiate PMU CORE/UNCORE devices.
> >>
> >> commit:
> >> 66ec1191 ("perf pmu: Unbreak perf record for arm/arm64 with events with
> >> explicit PMU")
> >>
> >> has introduced pmu->is_uncore, which is set to PMU UNCORE devices only.
> >> Adding changes to use pmu->is_uncore to identify UNCORE devices.
> >>
> >> Acked-by: Will Deacon <will.deacon@xxxxxxx>
> >> Tested-by: Shaokun Zhang <zhangshaokun@xxxxxxxxxxxxx>
> >> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@xxxxxxxxxx>
> >> ---
> >> tools/perf/util/pmu.c | 11 +++++++----
> >> 1 file changed, 7 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> >> index 8b17db5..9110718 100644
> >> --- a/tools/perf/util/pmu.c
> >> +++ b/tools/perf/util/pmu.c
> >> @@ -603,7 +603,6 @@ static void pmu_add_cpu_aliases(struct list_head
> >> *head, struct perf_pmu *pmu)
> >> */
> >> i = 0;
> >> while (1) {
> >> - const char *pname;
> >> pe = &map->table[i++];
> >> if (!pe->name) {
> >> @@ -612,9 +611,13 @@ static void pmu_add_cpu_aliases(struct list_head
> >> *head, struct perf_pmu *pmu)
> >> break;
> >> }
> >> - pname = pe->pmu ? pe->pmu : "cpu";
> >> - if (strncmp(pname, name, strlen(pname)))
> >> - continue;
> >> + if (pmu->is_uncore) {
> >> + /* check for uncore devices */
> >> + if (pe->pmu == NULL)
> >> + continue;
> >> + if (strncmp(pe->pmu, name, strlen(pe->pmu)))
> >> + continue;
> >> + }
> >> /* need type casts to override 'const' */
> >> __perf_pmu__new_alias(head, NULL, (char *)pe->name,
> >>
> >
>
> thanks
> Ganapat