From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752687AbdLEN6l (ORCPT ); Tue, 5 Dec 2017 08:58:41 -0500 Received: from mail.kernel.org ([198.145.29.99]:43626 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750810AbdLEN6j (ORCPT ); Tue, 5 Dec 2017 08:58:39 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B75A02190A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=acme@kernel.org Date: Tue, 5 Dec 2017 10:58:35 -0300 From: Arnaldo Carvalho de Melo To: Ganapatrao Kulkarni Cc: "Jin, Yao" , Ganapatrao Kulkarni , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Will Deacon , catalin.marinas@arm.com, mark.rutland@arm.com, Alexander Shishkin , Peter Zijlstra , Ingo Molnar , jnair@caviumnetworks.com, Zhangshaokun , Jonathan.Cameron@huawei.com, Robert Richter , "Jin, Yao" Subject: Re: [PATCH v9 3/5] perf utils: use pmu->is_uncore to detect PMU UNCORE devices Message-ID: <20171205135835.GD28405@kernel.org> References: <20171016183222.25750-1-ganapatrao.kulkarni@cavium.com> <20171016183222.25750-4-ganapatrao.kulkarni@cavium.com> <6fdad34d-1612-0447-e58e-5c748f92668d@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 > > 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 inst_retired.any > > vmstat-1754 1,609,892 cycles > > vmstat-1754 1,945,589 inst_retired.any > > vmstat-1754 inst_retired.any > > vmstat-1754 1,609,892 cpu_clk_unhalted.thread > > vmstat-1754 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 > >> Tested-by: Shaokun Zhang > >> Signed-off-by: Ganapatrao Kulkarni > >> --- > >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: acme@kernel.org (Arnaldo Carvalho de Melo) Date: Tue, 5 Dec 2017 10:58:35 -0300 Subject: [PATCH v9 3/5] perf utils: use pmu->is_uncore to detect PMU UNCORE devices In-Reply-To: References: <20171016183222.25750-1-ganapatrao.kulkarni@cavium.com> <20171016183222.25750-4-ganapatrao.kulkarni@cavium.com> <6fdad34d-1612-0447-e58e-5c748f92668d@linux.intel.com> Message-ID: <20171205135835.GD28405@kernel.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 at 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 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 > > 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 at 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 at 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 inst_retired.any > > vmstat-1754 1,609,892 cycles > > vmstat-1754 1,945,589 inst_retired.any > > vmstat-1754 inst_retired.any > > vmstat-1754 1,609,892 cpu_clk_unhalted.thread > > vmstat-1754 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 > >> Tested-by: Shaokun Zhang > >> Signed-off-by: Ganapatrao Kulkarni > >> --- > >> 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