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 <liuqi115@xxxxxxxxxx>
> >>
> >> 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 <leo.yan@xxxxxxxxxx>
> >> Signed-off-by: Qi Liu <liuqi115@xxxxxxxxxx>
> >> Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> >
> > 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 <john.garry@xxxxxxxxxx>
Ok, applied it now.
- Arnaldo
> Thanks! May I have the ack for the other 2 patches as well?
>
> Some replies below.
>
> >
> >> ---
> >> tools/perf/arch/arm/util/auxtrace.c | 63 +++++++++
> >> tools/perf/arch/arm/util/pmu.c | 3 +
> >> tools/perf/arch/arm64/util/Build | 2 +-
> >> tools/perf/arch/arm64/util/hisi-ptt.c | 188 ++++++++++++++++++++++++++
> >> tools/perf/util/auxtrace.c | 1 +
> >> tools/perf/util/auxtrace.h | 1 +
> >> tools/perf/util/hisi-ptt.h | 16 +++
> >> 7 files changed, 273 insertions(+), 1 deletion(-)
> >> create mode 100644 tools/perf/arch/arm64/util/hisi-ptt.c
> >> 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 @@
> >> * Author: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> >> */
> >> +#include <dirent.h>
> >> #include <stdbool.h>
> >> #include <linux/coresight-pmu.h>
> >> #include <linux/zalloc.h>
> >> +#include <api/fs/fs.h>
> >> #include "../../../util/auxtrace.h"
> >> #include "../../../util/debug.h"
> >> @@ -14,6 +16,7 @@
> >> #include "../../../util/pmu.h"
> >> #include "cs-etm.h"
> >> #include "arm-spe.h"
> >> +#include "hisi-ptt.h"
> >> static struct perf_pmu **find_all_arm_spe_pmus(int *nr_spes, int *err)
> >> {
> >> @@ -50,6 +53,52 @@ static struct perf_pmu **find_all_arm_spe_pmus(int *nr_spes, int *err)
> >> return arm_spe_pmus;
> >> }
> >> +static struct perf_pmu **find_all_hisi_ptt_pmus(int *nr_ptts, int *err)
> >> +{
> >> + const char *sysfs = sysfs__mountpoint();
> >> + struct perf_pmu **hisi_ptt_pmus = NULL;
> >> + struct dirent *dent;
> >> + char path[PATH_MAX];
> >> + DIR *dir = NULL;
> >> + int idx = 0;
> >> +
> >> + snprintf(path, PATH_MAX, "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
> >> + dir = opendir(path);
> >> + if (!dir) {
> >> + pr_err("can't read directory '%s'\n", EVENT_SOURCE_DEVICE_PATH);
> >> + *err = -EINVAL;
> >> + goto out;
> >
> > Do you really need to call closedir() in this scenario?
> >
>
> It's unnecessary here. will return NULL directly.
>
> >> + }
> >> +
> >> + while ((dent = readdir(dir))) {
> >> + if (strstr(dent->d_name, HISI_PTT_PMU_NAME))
> >> + (*nr_ptts)++;
> >> + }
> >> +
> >> + if (!(*nr_ptts))
> >> + goto out;
> >> +
> >> + hisi_ptt_pmus = zalloc(sizeof(struct perf_pmu *) * (*nr_ptts));
> >> + if (!hisi_ptt_pmus) {
> >> + pr_err("hisi_ptt alloc failed\n");
> >> + *err = -ENOMEM;
> >> + goto out;
> >> + }
> >> +
> >> + rewinddir(dir);
> >> + while ((dent = readdir(dir))) {
> >> + if (strstr(dent->d_name, HISI_PTT_PMU_NAME) && idx < (*nr_ptts)) {
> >
> > idx < *nr_ptts
> >
> > is ok
> >
>
> ok.
>
> >> + hisi_ptt_pmus[idx] = perf_pmu__find(dent->d_name);
> >> + if (hisi_ptt_pmus[idx])
> >> + idx++;
> >> + }
> >> + }
> >> +
> >> +out:
> >> + closedir(dir);
> >> + 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 * 1024)
> 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 {
> >> + struct auxtrace_record itr;
> >> + struct perf_pmu *hisi_ptt_pmu;
> >> + 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 perf_session *session,
> >> case PERF_AUXTRACE_S390_CPUMSF:
> >> err = s390_cpumsf_process_auxtrace_info(event, session);
> >> break;
> >> + case PERF_AUXTRACE_HISI_PTT:
> >> case PERF_AUXTRACE_UNKNOWN:
> >> default:
> >> 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 {
> >> PERF_AUXTRACE_CS_ETM,
> >> PERF_AUXTRACE_ARM_SPE,
> >> PERF_AUXTRACE_S390_CPUMSF,
> >> + PERF_AUXTRACE_HISI_PTT,
> >> };
> >> 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 1x .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/perf/
> 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 "hisi_ptt"
> >> +#define HISI_PTT_AUXTRACE_PRIV_SIZE sizeof(u64)
> >> +
> >> +struct auxtrace_record *hisi_ptt_recording_init(int *err,
> >> + struct perf_pmu *hisi_ptt_pmu);
> >> +
> >> +#endif
> >
> > .
--
- Arnaldo
