From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 6682DC4332F for ; Fri, 14 Oct 2022 20:43:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=cFMGAi9bK+b9WewQc0kP6+RS1Ek8Cpxp3yTciiHUmxo=; b=F3Jsmo6MfFkFq/ Gq7C/VH7uh6bur//kt5k5QF7x1sEntxx/9f18RmCfDTpf9DFuWAXXap7AWjBupUFHcs9I4jhI6S+D ercksR4k29x46AAggZhfs38vSxDD6qepiET+hOoMImZgmeUCftxpJ2+6ZZkwbHu2r80qGC8e7MGk2 6oSf5FcK8AMdNm8xEUEciMYDElH4Lq+JhX7qqaAfriYG1cbB2Ye3l1C6m2HYAcBDaOeP7gUlU5aVn UbfAktRot/DhTEZCsML3E9VL2aKRiU5mWH3KYyahPh8REvkoZ/wujf3jaoHDQjeSmG5C7GtLLAeQI SUgBP8Z7GAGiuxeLdreA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ojRVR-00FrQn-OQ; Fri, 14 Oct 2022 20:42:09 +0000 Received: from desiato.infradead.org ([2001:8b0:10b:1:d65d:64ff:fe57:4e05]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ojRVP-00FrOj-GK for linux-arm-kernel@bombadil.infradead.org; Fri, 14 Oct 2022 20:42:07 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=desiato.20200630; h=In-Reply-To:Content-Transfer-Encoding: Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: Sender:Reply-To:Content-ID:Content-Description; bh=iGX/VshnjepB1p91G/9kT0vALwqKDVXKAfVCzxKQuj8=; b=oQS3sgLmrOy0+xW3hRkO7yOLoO Jltbo4wgdECRdqlucvj0ZKraSZ+CCpO7wUT0PdMG76hdm2ZGIZn6frIcTGSXsLbA11W59KW56WlIq 6vJ6XT0GmRYvf3ZC8TggTc2nlj/hcZQJIAHmpOpQ4rR+07KmcX5faM1PQU+UuxcsrdLJGCVHmPiEI XfNXMthj9lMRWOYDVQ0CgHtloF9BzHcZQm2/DcIua1Axt7JSFNMdAKRZvxGxgwIR0ptqDWPM4jkA/ rPDR1MbZBAjh9SBT8M9Al15vv3WSQVSEZkf/g8s9piLaGPUALt1DD2812O4Axw/z/k71mxDBvaGye hUf2/5/w==; Received: from [187.19.238.93] (helo=quaco.ghostprotocols.net) by desiato.infradead.org with esmtpsa (Exim 4.94.2 #2 (Red Hat Linux)) id 1ojRVL-003Uyu-Ns; Fri, 14 Oct 2022 20:42:04 +0000 Received: by quaco.ghostprotocols.net (Postfix, from userid 1000) id 4A0BC4062C; Fri, 14 Oct 2022 17:42:01 -0300 (-03) Date: Fri, 14 Oct 2022 17:42:01 -0300 From: Arnaldo Carvalho de Melo To: Yicong Yang Cc: John Garry , yangyicong@hisilicon.com, gregkh@linuxfoundation.org, mathieu.poirier@linaro.org, will@kernel.org, james.clark@arm.com, leo.yan@linaro.org, alexander.shishkin@linux.intel.com, peterz@infradead.org, helgaas@kernel.org, lorenzo.pieralisi@arm.com, shameerali.kolothum.thodi@huawei.com, mark.rutland@arm.com, suzuki.poulose@arm.com, mingo@redhat.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org, linux-perf-users@vger.kernel.org, prime.zeng@huawei.com, zhangshaokun@hisilicon.com, linuxarm@huawei.com, liuqi6124@gmail.com, jonathan.cameron@huawei.com, mike.leach@linaro.org Subject: Re: [PATCH v13 2/3] perf tool: Add support for HiSilicon PCIe Tune and Trace device driver Message-ID: References: <20220919090045.6778-1-yangyicong@huawei.com> <20220919090045.6778-3-yangyicong@huawei.com> <77226dcb-d158-a4f7-8afb-ac89c07bed97@huawei.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <77226dcb-d158-a4f7-8afb-ac89c07bed97@huawei.com> X-Url: http://acmel.wordpress.com X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Em Mon, Sep 26, 2022 at 02:53:34PM +0800, Yicong Yang escreveu: > On 2022/9/23 18:49, John Garry wrote: > > On 19/09/2022 10:00, Yicong Yang wrote: > >> From: Qi Liu > >> > >> HiSilicon PCIe tune and trace device (PTT) could dynamically tune > >> the PCIe link's events, and trace the TLP headers). > >> > >> This patch add support for PTT device in perf tool, so users could > >> use 'perf record' to get TLP headers trace data. > >> > >> Reviewed-by: Leo Yan > >> Signed-off-by: Qi Liu > >> Signed-off-by: Yicong Yang > > = > > I'm not overly fimilar with the auxtrace code, but this patch looks ok = apart from comments, so acked-by seems more appropiate than reviewed-by: > > = > > Acked-by: John Garry Ok, applied it now. - Arnaldo = > Thanks! May I have the ack for the other 2 patches as well? > = > Some replies below. > = > > = > >> --- > >> =A0 tools/perf/arch/arm/util/auxtrace.c=A0=A0 |=A0 63 +++++++++ > >> =A0 tools/perf/arch/arm/util/pmu.c=A0=A0=A0=A0=A0=A0=A0 |=A0=A0 3 + > >> =A0 tools/perf/arch/arm64/util/Build=A0=A0=A0=A0=A0 |=A0=A0 2 +- > >> =A0 tools/perf/arch/arm64/util/hisi-ptt.c | 188 ++++++++++++++++++++++= ++++ > >> =A0 tools/perf/util/auxtrace.c=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 |=A0= =A0 1 + > >> =A0 tools/perf/util/auxtrace.h=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 |=A0= =A0 1 + > >> =A0 tools/perf/util/hisi-ptt.h=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 |=A0 1= 6 +++ > >> =A0 7 files changed, 273 insertions(+), 1 deletion(-) > >> =A0 create mode 100644 tools/perf/arch/arm64/util/hisi-ptt.c > >> =A0 create mode 100644 tools/perf/util/hisi-ptt.h > >> > >> diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm= /util/auxtrace.c > >> index 384c7cfda0fd..129ed72391a4 100644 > >> --- a/tools/perf/arch/arm/util/auxtrace.c > >> +++ b/tools/perf/arch/arm/util/auxtrace.c > >> @@ -4,9 +4,11 @@ > >> =A0=A0 * Author: Mathieu Poirier > >> =A0=A0 */ > >> =A0 +#include > >> =A0 #include > >> =A0 #include > >> =A0 #include > >> +#include > >> =A0 =A0 #include "../../../util/auxtrace.h" > >> =A0 #include "../../../util/debug.h" > >> @@ -14,6 +16,7 @@ > >> =A0 #include "../../../util/pmu.h" > >> =A0 #include "cs-etm.h" > >> =A0 #include "arm-spe.h" > >> +#include "hisi-ptt.h" > >> =A0 =A0 static struct perf_pmu **find_all_arm_spe_pmus(int *nr_spes, i= nt *err) > >> =A0 { > >> @@ -50,6 +53,52 @@ static struct perf_pmu **find_all_arm_spe_pmus(int = *nr_spes, int *err) > >> =A0=A0=A0=A0=A0 return arm_spe_pmus; > >> =A0 } > >> =A0 +static struct perf_pmu **find_all_hisi_ptt_pmus(int *nr_ptts, int= *err) > >> +{ > >> +=A0=A0=A0 const char *sysfs =3D sysfs__mountpoint(); > >> +=A0=A0=A0 struct perf_pmu **hisi_ptt_pmus =3D NULL; > >> +=A0=A0=A0 struct dirent *dent; > >> +=A0=A0=A0 char path[PATH_MAX]; > >> +=A0=A0=A0 DIR *dir =3D NULL; > >> +=A0=A0=A0 int idx =3D 0; > >> + > >> +=A0=A0=A0 snprintf(path, PATH_MAX, "%s" EVENT_SOURCE_DEVICE_PATH, sys= fs); > >> +=A0=A0=A0 dir =3D opendir(path); > >> +=A0=A0=A0 if (!dir) { > >> +=A0=A0=A0=A0=A0=A0=A0 pr_err("can't read directory '%s'\n", EVENT_SOU= RCE_DEVICE_PATH); > >> +=A0=A0=A0=A0=A0=A0=A0 *err =3D -EINVAL; > >> +=A0=A0=A0=A0=A0=A0=A0 goto out; > > = > > Do you really need to call closedir() in this scenario? > > = > = > It's unnecessary here. will return NULL directly. > = > >> +=A0=A0=A0 } > >> + > >> +=A0=A0=A0 while ((dent =3D readdir(dir))) { > >> +=A0=A0=A0=A0=A0=A0=A0 if (strstr(dent->d_name, HISI_PTT_PMU_NAME)) > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 (*nr_ptts)++; > >> +=A0=A0=A0 } > >> + > >> +=A0=A0=A0 if (!(*nr_ptts)) > >> +=A0=A0=A0=A0=A0=A0=A0 goto out; > >> + > >> +=A0=A0=A0 hisi_ptt_pmus =3D zalloc(sizeof(struct perf_pmu *) * (*nr_p= tts)); > >> +=A0=A0=A0 if (!hisi_ptt_pmus) { > >> +=A0=A0=A0=A0=A0=A0=A0 pr_err("hisi_ptt alloc failed\n"); > >> +=A0=A0=A0=A0=A0=A0=A0 *err =3D -ENOMEM; > >> +=A0=A0=A0=A0=A0=A0=A0 goto out; > >> +=A0=A0=A0 } > >> + > >> +=A0=A0=A0 rewinddir(dir); > >> +=A0=A0=A0 while ((dent =3D readdir(dir))) { > >> +=A0=A0=A0=A0=A0=A0=A0 if (strstr(dent->d_name, HISI_PTT_PMU_NAME) && = idx < (*nr_ptts)) { > > = > > idx < *nr_ptts > > = > > is ok > > = > = > ok. > = > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 hisi_ptt_pmus[idx] =3D perf_pmu__fi= nd(dent->d_name); > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (hisi_ptt_pmus[idx]) > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 idx++; > >> +=A0=A0=A0=A0=A0=A0=A0 } > >> +=A0=A0=A0 } > >> + > >> +out: > >> +=A0=A0=A0 closedir(dir); > >> +=A0=A0=A0 return hisi_ptt_pmus; > >> +} > >> + > = > [...] > = > >> + > >> +#define KiB(x) ((x) * 1024) > >> +#define MiB(x) ((x) * 1024 * 1024) > > = > > surely we have defines for these available elsewhere > > = > = > Yes but we looks like have no public definition for these: > = > yangyicong@ubuntu:~/mainline_linux/linux$ egrep -rwn "#define KiB|#define= MiB" tools/perf/ > tools/perf/arch/arm64/util/hisi-ptt.c:27:#define KiB(x) ((x) * 1024) > tools/perf/arch/arm64/util/hisi-ptt.c:28:#define MiB(x) ((x) * 1024 * 102= 4) > tools/perf/arch/arm64/util/arm-spe.c:28:#define KiB(x) ((x) * 1024) > tools/perf/arch/arm64/util/arm-spe.c:29:#define MiB(x) ((x) * 1024 * 1024) > tools/perf/arch/x86/util/intel-bts.c:28:#define KiB(x) ((x) * 1024) > tools/perf/arch/x86/util/intel-bts.c:29:#define MiB(x) ((x) * 1024 * 1024) > tools/perf/arch/x86/util/intel-pt.c:35:#define KiB(x) ((x) * 1024) > tools/perf/arch/x86/util/intel-pt.c:36:#define MiB(x) ((x) * 1024 * 1024) > tools/perf/util/cs-etm.h:188:#define KiB(x) ((x) * 1024) > tools/perf/util/cs-etm.h:189:#define MiB(x) ((x) * 1024 * 1024) > = > We can centralize this definition after this series. > = > >> + > >> +struct hisi_ptt_recording { > >> +=A0=A0=A0 struct auxtrace_record=A0=A0=A0 itr; > >> +=A0=A0=A0 struct perf_pmu *hisi_ptt_pmu; > >> +=A0=A0=A0 struct evlist *evlist; > >> +}; > >> + > = > [...] > = > >> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c > >> index 6edab8a16de6..c30611d9ee99 100644 > >> --- a/tools/perf/util/auxtrace.c > >> +++ b/tools/perf/util/auxtrace.c > >> @@ -1319,6 +1319,7 @@ int perf_event__process_auxtrace_info(struct per= f_session *session, > >> =A0=A0=A0=A0=A0 case PERF_AUXTRACE_S390_CPUMSF: > >> =A0=A0=A0=A0=A0=A0=A0=A0=A0 err =3D s390_cpumsf_process_auxtrace_info(= event, session); > >> =A0=A0=A0=A0=A0=A0=A0=A0=A0 break; > >> +=A0=A0=A0 case PERF_AUXTRACE_HISI_PTT: > >> =A0=A0=A0=A0=A0 case PERF_AUXTRACE_UNKNOWN: > >> =A0=A0=A0=A0=A0 default: > >> =A0=A0=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > >> diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h > >> index 6a4fbfd34c6b..3a122fc01ccd 100644 > >> --- a/tools/perf/util/auxtrace.h > >> +++ b/tools/perf/util/auxtrace.h > >> @@ -48,6 +48,7 @@ enum auxtrace_type { > >> =A0=A0=A0=A0=A0 PERF_AUXTRACE_CS_ETM, > >> =A0=A0=A0=A0=A0 PERF_AUXTRACE_ARM_SPE, > >> =A0=A0=A0=A0=A0 PERF_AUXTRACE_S390_CPUMSF, > >> +=A0=A0=A0 PERF_AUXTRACE_HISI_PTT, > >> =A0 }; > >> =A0 =A0 enum itrace_period_type { > >> diff --git a/tools/perf/util/hisi-ptt.h b/tools/perf/util/hisi-ptt.h > > = > > I'm still not a fan of having a header file which is only included by 1= x .c file, which this seems to be > > = > = > The stuff in hisi-ptt.h is shared across several source files: > = > yangyicong@ubuntu:~/mainline_linux/linux$ egrep -rn "hisi-ptt.h" tools/pe= rf/ > tools/perf/arch/arm64/util/hisi-ptt.c:21:#include "../../../util/hisi-ptt= .h" > tools/perf/arch/arm/util/auxtrace.c:19:#include "hisi-ptt.h" > tools/perf/arch/arm/util/pmu.c:13:#include "hisi-ptt.h" > = > Thanks, > Yicong > = > >> new file mode 100644 > >> index 000000000000..82283c81b4c1 > >> --- /dev/null > >> +++ b/tools/perf/util/hisi-ptt.h > >> @@ -0,0 +1,16 @@ > >> +/* SPDX-License-Identifier: GPL-2.0 */ > >> +/* > >> + * HiSilicon PCIe Trace and Tuning (PTT) support > >> + * Copyright (c) 2022 HiSilicon Technologies Co., Ltd. > >> + */ > >> + > >> +#ifndef INCLUDE__PERF_HISI_PTT_H__ > >> +#define INCLUDE__PERF_HISI_PTT_H__ > >> + > >> +#define HISI_PTT_PMU_NAME=A0=A0=A0=A0=A0=A0=A0 "hisi_ptt" > >> +#define HISI_PTT_AUXTRACE_PRIV_SIZE=A0=A0=A0 sizeof(u64) > >> + > >> +struct auxtrace_record *hisi_ptt_recording_init(int *err, > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= struct perf_pmu *hisi_ptt_pmu); > >> + > >> +#endif > > = > > . -- = - Arnaldo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel