Date:   Tue, 28 Nov 2023 22:24:08 -0800
From:   Ian Rogers <irogers@...gle.com>
To:     kan.liang@...ux.intel.com
Cc:     acme@...nel.org, mingo@...hat.com, peterz@...radead.org,
        mark.rutland@....com, namhyung@...nel.org, jolsa@...nel.org,
        adrian.hunter@...el.com, ravi.bangoria@....com,
        linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        Ammy Yi <ammy.yi@...el.com>
Subject: Re: [PATCH] perf mem: Fix perf mem error on hybrid

On Tue, Nov 28, 2023 at 12:39 PM <kan.liang@...ux.intel.com> wrote:
>
> From: Kan Liang <kan.liang@...ux.intel.com>
>
> The below error can be triggered on a hybrid machine.
>
>  $ perf mem record -t load sleep 1
>  event syntax error: 'breakpoint/mem-loads,ldlat=30/P'
>                                 \___ Bad event or PMU
>
>  Unable to find PMU or event on a PMU of 'breakpoint'
>
> In the perf_mem_events__record_args(), the current perf never checks the
> availability of a mem event on a given PMU. All the PMUs will be added
> to the perf mem event list. Perf errors out for the unsupported PMU.
>
> Extend perf_mem_event__supported() and take a PMU into account. Check
> the mem event for each PMU before adding it to the perf mem event list.
>
> Optimize the perf_mem_events__init() a little bit. The function is to
> check whether the mem events are supported in the system. It doesn't
> need to scan all PMUs. Just return with the first supported PMU is good
> enough.
>
> Fixes: 5752c20f3787 ("perf mem: Scan all PMUs instead of just core ones")
> Reported-by: Ammy Yi <ammy.yi@...el.com>
> Tested-by: Ammy Yi <ammy.yi@...el.com>
> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
> ---
>  tools/perf/util/mem-events.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
> index 954b235e12e5..3a2e3687878c 100644
> --- a/tools/perf/util/mem-events.c
> +++ b/tools/perf/util/mem-events.c
> @@ -100,11 +100,14 @@ int perf_mem_events__parse(const char *str)
>         return -1;
>  }
>
> -static bool perf_mem_event__supported(const char *mnt, char *sysfs_name)
> +static bool perf_mem_event__supported(const char *mnt, struct perf_pmu *pmu,
> +                                     struct perf_mem_event *e)
>  {
> +       char sysfs_name[100];
>         char path[PATH_MAX];
>         struct stat st;
>
> +       scnprintf(sysfs_name, sizeof(sysfs_name), e->sysfs_name, pmu->name);

Not sure if this is right. Looking at sysfs_name values:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/mem-events.c?h=perf-tools-next#n23
"cpu/events/mem-loads" and "cpu/events/mem-stores", so won't pmu->name
never be used?
Is there a missed change to change the cpu to %s?

Thanks,
Ian

>         scnprintf(path, PATH_MAX, "%s/devices/%s", mnt, sysfs_name);
>         return !stat(path, &st);
>  }
> @@ -120,7 +123,6 @@ int perf_mem_events__init(void)
>
>         for (j = 0; j < PERF_MEM_EVENTS__MAX; j++) {
>                 struct perf_mem_event *e = perf_mem_events__ptr(j);
> -               char sysfs_name[100];
>                 struct perf_pmu *pmu = NULL;
>
>                 /*
> @@ -136,12 +138,12 @@ int perf_mem_events__init(void)
>                  * of core PMU.
>                  */
>                 while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> -                       scnprintf(sysfs_name, sizeof(sysfs_name), e->sysfs_name, pmu->name);
> -                       e->supported |= perf_mem_event__supported(mnt, sysfs_name);
> +                       e->supported |= perf_mem_event__supported(mnt, pmu, e);
> +                       if (e->supported) {
> +                               found = true;
> +                               break;
> +                       }
>                 }
> -
> -               if (e->supported)
> -                       found = true;
>         }
>
>         return found ? 0 : -ENOENT;
> @@ -167,13 +169,10 @@ static void perf_mem_events__print_unsupport_hybrid(struct perf_mem_event *e,
>                                                     int idx)
>  {
>         const char *mnt = sysfs__mount();
> -       char sysfs_name[100];
>         struct perf_pmu *pmu = NULL;
>
>         while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> -               scnprintf(sysfs_name, sizeof(sysfs_name), e->sysfs_name,
> -                         pmu->name);
> -               if (!perf_mem_event__supported(mnt, sysfs_name)) {
> +               if (!perf_mem_event__supported(mnt, pmu, e)) {
>                         pr_err("failed: event '%s' not supported\n",
>                                perf_mem_events__name(idx, pmu->name));
>                 }
> @@ -183,6 +182,7 @@ static void perf_mem_events__print_unsupport_hybrid(struct perf_mem_event *e,
>  int perf_mem_events__record_args(const char **rec_argv, int *argv_nr,
>                                  char **rec_tmp, int *tmp_nr)
>  {
> +       const char *mnt = sysfs__mount();
>         int i = *argv_nr, k = 0;
>         struct perf_mem_event *e;
>
> @@ -211,6 +211,9 @@ int perf_mem_events__record_args(const char **rec_argv, int *argv_nr,
>                         while ((pmu = perf_pmus__scan(pmu)) != NULL) {
>                                 const char *s = perf_mem_events__name(j, pmu->name);
>
> +                               if (!perf_mem_event__supported(mnt, pmu, e))
> +                                       continue;
> +
>                                 rec_argv[i++] = "-e";
>                                 if (s) {
>                                         char *copy = strdup(s);
> --
> 2.35.1
>

Powered by blists - more mailing lists