| Message ID | CAKTKpr7oM4h_gxj1P=r_J_T5L_Mmtqqj_Uvji-fARKnQRnCP1w@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, I applied the diff but it's failed. jinyao@skl:~/skl-ws/perf-dev/lck-4594/src$ patch -p1 < 1.pat patching file tools/perf/util/pmu.c patch: **** malformed patch at line 41: *head, struct perf_pmu *pmu) Could you send the patch as attachment to me in another mail thread? to yao.jin@linux.intel.com cc yao.jin@intel.com Thanks Jin Yao On 12/5/2017 3:12 PM, Ganapatrao Kulkarni wrote: > 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;
A quick test with the new patch 'fix_json_v9_2.patch' shows it working.
See the log:
root@skl:/tmp# perf stat --per-thread -p 10322 -M CPI,IPC
^C
Performance counter stats for process id '10322':
vmstat-10322 1,879,654 inst_retired.any
# 0.8 CPI
vmstat-10322 1,565,807 cycles
vmstat-10322 1,879,654 inst_retired.any
# 1.2 IPC
vmstat-10322 1,565,807 cpu_clk_unhalted.thread
2.850291804 seconds time elapsed
Thanks for fixing it quickly.
Thanks
Jin Yao
On 12/5/2017 3:23 PM, Jin, Yao wrote:
> Hi,
>
> I applied the diff but it's failed.
>
> jinyao@skl:~/skl-ws/perf-dev/lck-4594/src$ patch -p1 < 1.pat
> patching file tools/perf/util/pmu.c
> patch: **** malformed patch at line 41: *head, struct perf_pmu *pmu)
>
> Could you send the patch as attachment to me in another mail thread?
>
> to yao.jin@linux.intel.com
> cc yao.jin@intel.com
>
> Thanks
> Jin Yao
>
> On 12/5/2017 3:12 PM, Ganapatrao Kulkarni wrote:
>> 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;
Em Tue, Dec 05, 2017 at 08:35:22PM +0800, Jin, Yao escreveu: > A quick test with the new patch 'fix_json_v9_2.patch' shows it working. Where is this fix_json_v9_2.patch file? I want to fold it with the patch introducing the problem. - Arnaldo > See the log: > > root@skl:/tmp# perf stat --per-thread -p 10322 -M CPI,IPC > ^C > Performance counter stats for process id '10322': > > vmstat-10322 1,879,654 inst_retired.any # > 0.8 CPI > vmstat-10322 1,565,807 cycles > vmstat-10322 1,879,654 inst_retired.any # > 1.2 IPC > vmstat-10322 1,565,807 cpu_clk_unhalted.thread > > 2.850291804 seconds time elapsed > > Thanks for fixing it quickly. > > Thanks > Jin Yao > > On 12/5/2017 3:23 PM, Jin, Yao wrote: > > Hi, > > > > I applied the diff but it's failed. > > > > jinyao@skl:~/skl-ws/perf-dev/lck-4594/src$ patch -p1 < 1.pat > > patching file tools/perf/util/pmu.c > > patch: **** malformed patch at line 41: *head, struct perf_pmu *pmu) > > > > Could you send the patch as attachment to me in another mail thread? > > > > to yao.jin@linux.intel.com > > cc yao.jin@intel.com > > > > Thanks > > Jin Yao > > > > On 12/5/2017 3:12 PM, Ganapatrao Kulkarni wrote: > > > 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;
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@linux.intel.com> 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@cavium.com> > > 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@arm.com> > >> Tested-by: Shaokun Zhang <zhangshaokun@hisilicon.com> > >> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com> > >> --- > >> 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
Hi Arnaldo, On Tue, Dec 5, 2017 at 7:26 PM, Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > Em Tue, Dec 05, 2017 at 08:35:22PM +0800, Jin, Yao escreveu: >> A quick test with the new patch 'fix_json_v9_2.patch' shows it working. > > Where is this fix_json_v9_2.patch file? I want to fold it with the patch > introducing the problem. I will send you formal patch ASAP. thanks Ganapat > > - Arnaldo > >> See the log: >> >> root@skl:/tmp# perf stat --per-thread -p 10322 -M CPI,IPC >> ^C >> Performance counter stats for process id '10322': >> >> vmstat-10322 1,879,654 inst_retired.any # >> 0.8 CPI >> vmstat-10322 1,565,807 cycles >> vmstat-10322 1,879,654 inst_retired.any # >> 1.2 IPC >> vmstat-10322 1,565,807 cpu_clk_unhalted.thread >> >> 2.850291804 seconds time elapsed >> >> Thanks for fixing it quickly. >> >> Thanks >> Jin Yao >> >> On 12/5/2017 3:23 PM, Jin, Yao wrote: >> > Hi, >> > >> > I applied the diff but it's failed. >> > >> > jinyao@skl:~/skl-ws/perf-dev/lck-4594/src$ patch -p1 < 1.pat >> > patching file tools/perf/util/pmu.c >> > patch: **** malformed patch at line 41: *head, struct perf_pmu *pmu) >> > >> > Could you send the patch as attachment to me in another mail thread? >> > >> > to yao.jin@linux.intel.com >> > cc yao.jin@intel.com >> > >> > Thanks >> > Jin Yao >> > >> > On 12/5/2017 3:12 PM, Ganapatrao Kulkarni wrote: >> > > 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; thanks Ganapat
Em Tue, Dec 05, 2017 at 08:35:22PM +0800, Jin, Yao escreveu: > A quick test with the new patch 'fix_json_v9_2.patch' shows it working. I'll take this as a Tested-by: you, ok? > See the log: > > root@skl:/tmp# perf stat --per-thread -p 10322 -M CPI,IPC > ^C > Performance counter stats for process id '10322': > > vmstat-10322 1,879,654 inst_retired.any # > 0.8 CPI > vmstat-10322 1,565,807 cycles > vmstat-10322 1,879,654 inst_retired.any # > 1.2 IPC > vmstat-10322 1,565,807 cpu_clk_unhalted.thread > > 2.850291804 seconds time elapsed > > Thanks for fixing it quickly. > > Thanks > Jin Yao > > On 12/5/2017 3:23 PM, Jin, Yao wrote: > > Hi, > > > > I applied the diff but it's failed. > > > > jinyao@skl:~/skl-ws/perf-dev/lck-4594/src$ patch -p1 < 1.pat > > patching file tools/perf/util/pmu.c > > patch: **** malformed patch at line 41: *head, struct perf_pmu *pmu) > > > > Could you send the patch as attachment to me in another mail thread? > > > > to yao.jin@linux.intel.com > > cc yao.jin@intel.com > > > > Thanks > > Jin Yao > > > > On 12/5/2017 3:12 PM, Ganapatrao Kulkarni wrote: > > > 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;
On 12/6/2017 2:42 AM, Arnaldo Carvalho de Melo wrote: > Em Tue, Dec 05, 2017 at 08:35:22PM +0800, Jin, Yao escreveu: >> A quick test with the new patch 'fix_json_v9_2.patch' shows it working. > > I'll take this as a Tested-by: you, ok? > Hi Arnaldo, I didn't do a full test for this patch and for the whole patch series. I just do a quick test and it shows that the regression issue which was found in 'perf stat --per-thread' test case is disappear. If you think it's enough for adding Tested-by, that's fine for me. :) Thanks Jin Yao >> See the log: >> >> root@skl:/tmp# perf stat --per-thread -p 10322 -M CPI,IPC >> ^C >> Performance counter stats for process id '10322': >> >> vmstat-10322 1,879,654 inst_retired.any # >> 0.8 CPI >> vmstat-10322 1,565,807 cycles >> vmstat-10322 1,879,654 inst_retired.any # >> 1.2 IPC >> vmstat-10322 1,565,807 cpu_clk_unhalted.thread >> >> 2.850291804 seconds time elapsed >> >> Thanks for fixing it quickly. >> >> Thanks >> Jin Yao >> >> On 12/5/2017 3:23 PM, Jin, Yao wrote: >>> Hi, >>> >>> I applied the diff but it's failed. >>> >>> jinyao@skl:~/skl-ws/perf-dev/lck-4594/src$ patch -p1 < 1.pat >>> patching file tools/perf/util/pmu.c >>> patch: **** malformed patch at line 41: *head, struct perf_pmu *pmu) >>> >>> Could you send the patch as attachment to me in another mail thread? >>> >>> to yao.jin@linux.intel.com >>> cc yao.jin@intel.com >>> >>> Thanks >>> Jin Yao >>> >>> On 12/5/2017 3:12 PM, Ganapatrao Kulkarni wrote: >>>> 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;
Em Wed, Dec 06, 2017 at 08:30:37AM +0800, Jin, Yao escreveu: > > > On 12/6/2017 2:42 AM, Arnaldo Carvalho de Melo wrote: > > Em Tue, Dec 05, 2017 at 08:35:22PM +0800, Jin, Yao escreveu: > > > A quick test with the new patch 'fix_json_v9_2.patch' shows it working. > > > > I'll take this as a Tested-by: you, ok? > > Hi Arnaldo, > > I didn't do a full test for this patch and for the whole patch series. > > I just do a quick test and it shows that the regression issue which was > found in 'perf stat --per-thread' test case is disappear. > > If you think it's enough for adding Tested-by, that's fine for me. :) I guess this addresses at least your previous regression report, so I think it is warranted, thanks! - arnaldo > Thanks > Jin Yao > > > > See the log: > > > > > > root@skl:/tmp# perf stat --per-thread -p 10322 -M CPI,IPC > > > ^C > > > Performance counter stats for process id '10322': > > > > > > vmstat-10322 1,879,654 inst_retired.any # > > > 0.8 CPI > > > vmstat-10322 1,565,807 cycles > > > vmstat-10322 1,879,654 inst_retired.any # > > > 1.2 IPC > > > vmstat-10322 1,565,807 cpu_clk_unhalted.thread > > > > > > 2.850291804 seconds time elapsed > > > > > > Thanks for fixing it quickly. > > > > > > Thanks > > > Jin Yao > > > > > > On 12/5/2017 3:23 PM, Jin, Yao wrote: > > > > Hi, > > > > > > > > I applied the diff but it's failed. > > > > > > > > jinyao@skl:~/skl-ws/perf-dev/lck-4594/src$ patch -p1 < 1.pat > > > > patching file tools/perf/util/pmu.c > > > > patch: **** malformed patch at line 41: *head, struct perf_pmu *pmu) > > > > > > > > Could you send the patch as attachment to me in another mail thread? > > > > > > > > to yao.jin@linux.intel.com > > > > cc yao.jin@intel.com > > > > > > > > Thanks > > > > Jin Yao > > > > > > > > On 12/5/2017 3:12 PM, Ganapatrao Kulkarni wrote: > > > > > 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;
On 12/6/2017 9:47 PM, Arnaldo Carvalho de Melo wrote: > Em Wed, Dec 06, 2017 at 08:30:37AM +0800, Jin, Yao escreveu: >> >> >> On 12/6/2017 2:42 AM, Arnaldo Carvalho de Melo wrote: >>> Em Tue, Dec 05, 2017 at 08:35:22PM +0800, Jin, Yao escreveu: >>>> A quick test with the new patch 'fix_json_v9_2.patch' shows it working. >>> >>> I'll take this as a Tested-by: you, ok? >> >> Hi Arnaldo, >> >> I didn't do a full test for this patch and for the whole patch series. >> >> I just do a quick test and it shows that the regression issue which was >> found in 'perf stat --per-thread' test case is disappear. >> >> If you think it's enough for adding Tested-by, that's fine for me. :) > > I guess this addresses at least your previous regression report, so I > think it is warranted, thanks! > > - arnaldo > That's fine, thanks! Thanks Jin Yao
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)
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. 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@linux.intel.com> 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@cavium.com> > 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@arm.com> >> Tested-by: Shaokun Zhang <zhangshaokun@hisilicon.com> >> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com> >> --- >> 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