Re: [PATCH] perf mem: Fix perf mem error on hybrid

Ian Rogers
2023-11-29T06:24:27Z

On Tue, Nov 28, 2023 at 12:39 PM <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=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@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->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
>