Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <20231128203940.3964287-1-kan.liang@linux.intel.com>
In-Reply-To: <20231128203940.3964287-1-kan.liang@linux.intel.com>
From:   Ian Rogers <irogers@google.com>
Date:   Tue, 28 Nov 2023 22:24:08 -0800
Message-ID: <CAP-5=fUdEgnwk_FNHb-Ju3wCYE2PLLrPHqwZoyBGyURXQhBeSA@mail.gmail.com>
Subject: Re: [PATCH] perf mem: Fix perf mem error on hybrid
To:     kan.liang@linux.intel.com
Cc:     acme@kernel.org, mingo@redhat.com, peterz@infradead.org,
        mark.rutland@arm.com, namhyung@kernel.org, jolsa@kernel.org,
        adrian.hunter@intel.com, ravi.bangoria@amd.com,
        linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        Ammy Yi <ammy.yi@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Nov 28, 2023 at 12:39=E2=80=AFPM <kan.liang@linux.intel.com> wrote:
>
> From: Kan Liang <kan.liang@linux.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=3D30/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@intel.com>
> Tested-by: Ammy Yi <ammy.yi@intel.com>
> Signed-off-by: Kan Liang <kan.liang@linux.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->nam=
e);

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/tr=
ee/tools/perf/util/mem-events.c?h=3Dperf-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 =3D 0; j < PERF_MEM_EVENTS__MAX; j++) {
>                 struct perf_mem_event *e =3D perf_mem_events__ptr(j);
> -               char sysfs_name[100];
>                 struct perf_pmu *pmu =3D NULL;
>
>                 /*
> @@ -136,12 +138,12 @@ int perf_mem_events__init(void)
>                  * of core PMU.
>                  */
>                 while ((pmu =3D perf_pmus__scan(pmu)) !=3D NULL) {
> -                       scnprintf(sysfs_name, sizeof(sysfs_name), e->sysf=
s_name, pmu->name);
> -                       e->supported |=3D perf_mem_event__supported(mnt, =
sysfs_name);
> +                       e->supported |=3D perf_mem_event__supported(mnt, =
pmu, e);
> +                       if (e->supported) {
> +                               found =3D true;
> +                               break;
> +                       }
>                 }
> -
> -               if (e->supported)
> -                       found =3D 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 =3D sysfs__mount();
> -       char sysfs_name[100];
>         struct perf_pmu *pmu =3D NULL;
>
>         while ((pmu =3D perf_pmus__scan(pmu)) !=3D 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(s=
truct 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 =3D sysfs__mount();
>         int i =3D *argv_nr, k =3D 0;
>         struct perf_mem_event *e;
>
> @@ -211,6 +211,9 @@ int perf_mem_events__record_args(const char **rec_arg=
v, int *argv_nr,
>                         while ((pmu =3D perf_pmus__scan(pmu)) !=3D NULL) =
{
>                                 const char *s =3D perf_mem_events__name(j=
, pmu->name);
>
> +                               if (!perf_mem_event__supported(mnt, pmu, =
e))
> +                                       continue;
> +
>                                 rec_argv[i++] =3D "-e";
>                                 if (s) {
>                                         char *copy =3D strdup(s);
> --
> 2.35.1
>